klimek added inline comments.
================
Comment at: include/clang/Format/Format.h:1375
+ std::vector<std::string> EnclosingFunctionNames;
+ /// \brief The canonical delimiter for this language.
+ std::string CanonicalDelimiter;
----------------
djasper wrote:
> krasimir wrote:
> > djasper wrote:
> > > 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).
> > I believe these should all go together: the reason that we propagate all
> > configured languages to the formatting library is to be able to use them as
> > a replacement for the BasedOnStyle in RawStringFormat. To make this
> > possible, we need to update the internal structure of RawStringFormat
> > itself to base it around each language. The canonical delimiter part is
> > just a convenience for this I guess, which could be split.
> >
> > My biggest concern with (1) is that since it has no visible behavior and no
> > other uses except for the adaptation of (2), it is not testable by itself
> > and it's not evident that a patch doing just (1) would handle the things
> > correctly.
> Ok, if you wish, this is not an unreasonable argument. But let's still do the
> code review in two steps: Lets for now just get the part of handling multiple
> languages straight and figure out the rest once we are sure that that part is
> fine.
>
> (I do think you can test it, though - but it depends on whether I can
> convince you to go with the FormatStyleSet approach ;) )
On a philosophical level, something that has no visible behavior, and just
restructures the code, should be tested by existing tests?
Enclosing function names also seems like an extra feature that could be pulled
out, btw.
================
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:
> krasimir wrote:
> > 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.
> But it's not just defaulting to LK_None what we are saying we are
> implementing. I think the documentation suggestion that we implement very
> basic inheritance. E.g. if the style for LK_None set the ColumnLimit to 42, I
> would expect that the styles for all other languages that don't explicitly
> set a ColumnLimit would also use 42. I don't think this is currently
> implemented and I don't think this patch implements it. But I think we should
> :).
>
> I agree that the FormatStyleSet approach would have some consequences, but I
> also think that it is much cleaner. Your current solution feels like we us
> working around technical debt and creating more technical debt to do it :(.
> Maybe Manuel has thoughts here?
I agree that we should test the inheritance if it's documented :)
I don't have super strong feelings where the logic of implementing that
inheritance lives - both the function that parses the data and the data
structure we hand around seem fine for that, as it's easy to change
That said, I do agree that FormatStyle being a recursive graph data structure
is weird and unexpected; I had to ask Daniel what he actually meant here, and
then went "how does this even work??!" before realizing that FormatStyle is
both a single style and also containing all other styles.
-> in conclusion, I agree with Daniel: we should have a FormatStyleSet and pass
that around everywhere. That's a pure refactoring that looks like it would
already make the current code better, so I also agree it should be done first
and be covered by existing tests.
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
