[PATCH] D65183: [Format] Make it easy to add new format::FormatStyle::LanguageStandard. NFCI

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

I haven't really understood D65043 <https://reviews.llvm.org/D65043> yet, but I think this is a good change either way.



================
Comment at: lib/Format/TokenAnnotator.cpp:2862
     return Right.is(TT_TemplateCloser) && Left.is(TT_TemplateCloser) &&
-           (Style.Standard != FormatStyle::LS_Cpp11 || Style.SpacesInAngles);
+           (Style.Standard == FormatStyle::LS_Cpp03 || Style.SpacesInAngles);
   }
----------------
modocache wrote:
> Just a note: I don't know what the original intent of https://github.com/llvm/llvm-project/commit/dd978ae0e11 was, but in D65043 I modified this condition to be `Style.Standard == FormatStyle::LS_Cpp03 || Style.Standard == FormatStyle::LS_Auto`, because I believe that mirrors the current behavior exactly. With this change, a user that specified a standard of `FormatStyle::LS_Auto` will experience a change in behavior.
If I'm reading the code right, `Formatter::analyze`calls `deriveLocalStyle` first, which replaces `LS_Auto` with a concrete style. Then later, it calls `TokenAnnotator::calculateFormattingInformation` which is what ultimately calls `spaceRequiredBefore`. So I think we never see `Auto` here, and the two forms are equivalent.


================
Comment at: lib/Format/TokenAnnotator.cpp:2862
     return Right.is(TT_TemplateCloser) && Left.is(TT_TemplateCloser) &&
-           (Style.Standard != FormatStyle::LS_Cpp11 || Style.SpacesInAngles);
+           (Style.Standard == FormatStyle::LS_Cpp03 || Style.SpacesInAngles);
   }
----------------
sammccall wrote:
> modocache wrote:
> > Just a note: I don't know what the original intent of https://github.com/llvm/llvm-project/commit/dd978ae0e11 was, but in D65043 I modified this condition to be `Style.Standard == FormatStyle::LS_Cpp03 || Style.Standard == FormatStyle::LS_Auto`, because I believe that mirrors the current behavior exactly. With this change, a user that specified a standard of `FormatStyle::LS_Auto` will experience a change in behavior.
> If I'm reading the code right, `Formatter::analyze`calls `deriveLocalStyle` first, which replaces `LS_Auto` with a concrete style. Then later, it calls `TokenAnnotator::calculateFormattingInformation` which is what ultimately calls `spaceRequiredBefore`. So I think we never see `Auto` here, and the two forms are equivalent.
nit: consider `< Cpp11`, or `<= Cpp03`? Fits in the spirit of this patch :-)


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65183/new/

https://reviews.llvm.org/D65183