[PATCH] D40909: [clang-format] Reorganize raw string delimiters

[Daniel Jasper via Phabricator via cfe-commits]

djasper added inline comments.


================
Comment at: include/clang/Format/Format.h:1216
+    LK_TextProto,
+    /// Do not use. Keep at last position.
+    LK_End,
----------------
Lets find a way to implement without this in the public header file.


================
Comment at: include/clang/Format/Format.h:1375
+    std::vector<std::string> EnclosingFunctionNames;
+    /// \brief The canonical delimiter for this language.
+    std::string CanonicalDelimiter;
----------------
Can you pull apart this patch?

In my view, it has three parts that have an ordering, but are actually fairly independent:

1. Propagate all configured languages to the formatting library. First patch to land, should not affect the visible behavior.
2. Restructure RawStringFormat to be centered around each language. This is a restructuring to make things easier and use #1.
3. Add a CanonicalDelimiter and make clang-format canonicalize it.

I'll focus my comments on what's required for #1 for now as that is already complicated (IMO).


================
Comment at: lib/Format/Format.cpp:919
+  }
+  if (LanguageFound) {
+    for (int i = Styles.size() - 1; i >= 0; --i) {
----------------
Prefer early exit, i.e.

  if (!LanguageFound)
    return make_error_code(ParseError::Unsuitable);
  ...


================
Comment at: lib/Format/Format.cpp:920
+  if (LanguageFound) {
+    for (int i = Styles.size() - 1; i >= 0; --i) {
+      if (Styles[i].Language == FormatStyle::LK_None) {
----------------
I think this is getting a bit convoluted and I don't even understand whether we are doing what is document (even before this patch).

So in lines 892-905, we verify that:
- Only the first Style in the file is allowed be LK_None.
- No language is duplicated.

That seems good.

According to the documentation: "The first section may have no language set, it will set the default style options for all lanugages.". Does the latter part actually happen? Seems to me that we are just setting "Style" to the style configured for a specific language, completely ignoring values that might have been set in the LK_None style. Or is that somehow happening when reading the JSON?

Independent of that, I think we should use this structure more explicitly. I think we should create an additional class (FormatStyles or FormatStyleSet or something) that is returned by this function and handed to the formatting library. This function then doesn't need to look at the language anymore.

That class should then have a function getFormatStyle(LanguageKind Language); that returns the style for a particular language (doing the default logic, etc.). Internally, it can likely just have a map<LK, Style> and I don't think you need to pre-fill that for all language kinds. If a language kind is not in the map, you can just return what's stored for LK_None. WDYT?


Repository:
  rC Clang

https://reviews.llvm.org/D40909