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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
