[PATCH] D79773: [clang-format] Improve clang-formats handling of concepts

[Michael Schellenberger Costa via Phabricator via cfe-commits]

miscco added a comment.

You are missing to check the boolean in `CHECK_PARSE_BOOL` in FormatTest.cpp



================
Comment at: clang/lib/Format/TokenAnnotator.cpp:1566
       Current.Type = TT_TrailingReturnArrow;
-
+    } else if (Current.is(tok::arrow) && Current.Previous &&
+               Current.Previous->is(tok::r_brace)) {
----------------
MyDeveloperDay wrote:
> miscco wrote:
> > Should that really be `tok::arrow` and not `tok::kw_requires`?
> This is to prevent the breaking between } and -> in the following (which could be some way from the requires
> 
> `{ t.foo(u) } -> typename T::foo_type;`
I believe this should carry some state that we are inside of an requires-expression. 


================
Comment at: clang/lib/Format/TokenAnnotator.cpp:3484
+  // concept ...
+  if (Left.is(TT_TemplateCloser) && Right.is(tok::kw_concept))
+    return true;
----------------
MyDeveloperDay wrote:
> miscco wrote:
> > I think we should have an option to allow short requires clauses on the same line similar to allow short functions
> Actually at present it will pull short requires onto a new line, this particular line is about keeping `concept` on a new line (most examplesI've seen are like this..i.e.
> 
> ```template<typename T>
> concept...```
> 
> rather than 
> 
> `template<typename T> concept...`
> 
> For me the natural thing here is to enforce a newline and then relax that later if we think it needs it, but I'm wary of adding too many options upfront. 
> 
> This is virgin territory as most style guides out there don't specify anything for concepts yet, to be honest i'd prefer to establish an LLVM style and wait for the other style guides to catch up. (I know we will revisit this later), just want to keep the initial commit small. (incremental development is encouraged in LLVM) and multiple options will be a huge commit that never gets passed.
> 
> 
Personally I agree that this is the natural style, at the same time there are configurations that allow short template declarations on a single line, which I believe would be similarly welcome. 


That said I agree adding this later is fine


================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:2284
+
+void UnwrappedLineParser::parseRequires() {
+  assert(FormatTok->Tok.is(tok::kw_requires) && "'requires' expected");
----------------
MyDeveloperDay wrote:
> miscco wrote:
> > I believe we should separate between constraint expressions aka: `tempalte<  > requires Concept1 && Concept2` and requires expressions aka `requires { ... }`
> > 
> > The two are quite different. That said the handling of requires expressions should be "simple" as we only need to find the matching "}". 
> > 
> > As an upside this would also handle the famous `requires requires`
> This is effectively the `if` at line 2305 seeing "requires {" or requires(Foo t) {" sets off down the parseBlock path which can itself include requires.
> 
> If you do have an example you think would break this please feel free to add that in a comment and I'll add it to the unit tests and rework accordingly.
> 
> My exposure to concepts is still pretty new, I'm not even sure I've covered all the places it can be used, but I wanted to start because at present I see alot of the MS STL(@STL_MSFT ) covered with // clang-format off around the concepts.
Yes that was also the reason I tried it out because I often added those // clang-format off snippets once too often 


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

https://reviews.llvm.org/D79773