[PATCH] D94500: Rework Whitesmiths mode to use line-level values in UnwrappedLineParser

[Tim Wojtulewicz via Phabricator via cfe-commits]

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