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

Krasimir Georgiev via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Dec 7 05:43:11 PST 2017


krasimir added inline comments.


================
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) {
----------------
djasper wrote:
> 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?
Yes, defaulting to the None for missing language specifications is handled at line 912:
```
    if (!LanguageFound && (Styles[i].Language == Language ||
                           Styles[i].Language == FormatStyle::LK_None
```

I was thinking of the FormatStyleSet approach but the problem is that this has repercussions all over the library. We could indeed update this specific function that way, but fundamentally the additional language styles are part of the FormatStyle and need to somehow be recorded inside there. That's why I went with KISS and directly made this function handle that case.


Repository:
  rC Clang

https://reviews.llvm.org/D40909





More information about the cfe-commits mailing list