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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay marked 6 inline comments as done.
MyDeveloperDay added a subscriber: STL_MSFT.
MyDeveloperDay added inline comments.


================
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)) {
----------------
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;`


================
Comment at: clang/lib/Format/TokenAnnotator.cpp:3484
+  // concept ...
+  if (Left.is(TT_TemplateCloser) && Right.is(tok::kw_concept))
+    return true;
----------------
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.




================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:2284
+
+void UnwrappedLineParser::parseRequires() {
+  assert(FormatTok->Tok.is(tok::kw_requires) && "'requires' expected");
----------------
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.


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

https://reviews.llvm.org/D79773