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

[Rihan Yang via Phabricator via cfe-commits]

air20 marked an inline comment as done.
air20 added inline comments.


================
Comment at: clang/lib/ASTMatchers/Dynamic/Registry.cpp:282
   REGISTER_MATCHER(hasInitStatement);
+  REGISTER_MATCHER(hasInitializer);
   REGISTER_MATCHER(hasKeywordSelector);
----------------
aaron.ballman wrote:
> 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.
 Reverted. But I thought the purpose of sorting is to make it look better and maintain *one* canonical order. Not a big deal anyhow.


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

https://reviews.llvm.org/D72233