timwoj added inline comments.

================
Comment at: clang/lib/Format/UnwrappedLineParser.h:143
   bool tryToParseSimpleAttribute();
-  void addUnwrappedLine();
+  void addUnwrappedLine(bool RemoveLevel = true);
   bool eof() const;
----------------
HazardyKnusperkeks wrote:
> A `bool` parameter for something like that is not very nice. An `enum 
> (class)` is much nicer.
> 
> If one does not know the declaration `addUnwrappedLine(false)` seems a bit 
> odd, `addUnwrappedLine(LineLevel::Keep)` looks better. (The naming may be 
> changed.)
Noted, I'll make it an enum in the next update.


================
Comment at: clang/unittests/Format/FormatTest.cpp:13707
                WhitesmithsBraceStyle);
 
   verifyFormat("enum X\n"
----------------
MyDeveloperDay wrote:
> timwoj wrote:
> > MyDeveloperDay wrote:
> > > any reason why this is being removed?
> > This is an artifact of there not being any sort of actual guide for 
> > Whitesmiths. I have it set now to always indent the case labels (and it 
> > should just ignore the IndentCaseLabels option entirely, like it does for 
> > IndentCaseBlocks). I can certainly fix that option to work again though.
> see the images from {D93806}, you are saying that anyone with 
> bracewrappingsytle of Whitesmith cannot decide if they can or cannot indent 
> case labels?
> 
> Could you show me a spec that show Whitesmiths is the other way around?
Part of the problem is that there isn't any sort of official spec for 
Whitesmiths that I've been able to find. I'm working on a project that uses it, 
and have been basically going off their layout as the proper way of doing 
things. Looking at those images, I agree that the option should continue to 
function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94500

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to