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