[PATCH] D72233: Add a new AST matcher 'optionally'.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/lib/ASTMatchers/Dynamic/Registry.cpp:282
   REGISTER_MATCHER(hasInitStatement);
+  REGISTER_MATCHER(hasInitializer);
   REGISTER_MATCHER(hasKeywordSelector);
----------------
air20 wrote:
> aaron.ballman wrote:
> > air20 wrote:
> > > aaron.ballman wrote:
> > > > Why did this one move? Please keep the list sorted alphabetically.
> > > I think this wasn’t sorted correctly before.
> > I think it was sorted properly before (`i` comes before `S` even if you ignore capitalization), but regardless, it's a change unrelated to the patch, which we usually ask to be a separate patch.
> I trust my Vim for this :) I think uppercase letters have a smaller ASCII value so they come first.
> 
> Line 354-355 are sorted correctly, for example.
> ```
> REGISTER_MATCHER(isConstQualified);
> REGISTER_MATCHER(isConstexpr);
> ```
> 
> I'd rather not upload another patch for this though.
> I trust my Vim for this :) I think uppercase letters have a smaller ASCII value so they come first.

I think it depends on your sort criteria and whether you are sorting this list for humans or computers. I sort it for humans where `i` comes before `s` alphabetically because the goal of alphabetizing this list is for a human to quickly scan it visually. (Computer sort order is unimportant in this case -- these are all being stored into a hash map where the order does not matter.)

> I'd rather not upload another patch for this though.

This is a change unrelated to the patch at hand, so it should be removed from this patch. If you feel strongly about changing this, please start a separate patch.


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

https://reviews.llvm.org/D72233