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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
