[PATCH] D87244: [clang] Add fix-it for -Wreorder-ctor.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

thanks, comments around some implementations. the only high level question i have is about the choice of location for fix-it (see the detailed comment inline)



================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:5198
+static bool RangeContainsComments(Sema &SemaRef, SourceLocation Begin,
+                                  SourceLocation End) {
+  auto &SrcMgr = SemaRef.getSourceManager();
----------------
nit: assert for fileids of begin and end at the beginning.


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:5208
+  while (!Lex.LexFromRawLexer(Tok) &&
+         SrcMgr.isBeforeInTranslationUnit(Tok.getLocation(), End)) {
+    if (Tok.is(tok::comment))
----------------
since Tok and End are in the same file you can just use `<` operator.


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:5298
+      if (IdealIndex == NumIdealInits) {
+        // This initializer is not in the ideal list at all. This can happen in
+        // a class with dependent base, when we cannot tell if initializer is
----------------
i don't really follow when this happens. comments mentions a dependent base, but test case only has a templated constructor. why do we think it is safe to continue in this case, while we bail out in case of a dependent class? i would suggest just bailing out at the beginning of the function if constructor is dependent too (in addition to it's class being dependent)


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:5311
+    // previous (valid) initializer, emit a warning.
+    if (IsOutOfOrder && !InitsWithIdealIndex.empty()) {
+      // Emit previous diagnostic, if any.
----------------
why not move this into previous if block i.e.

```
if (isoutoforder) {
 if(!Initswithidealindex.empty()) { ... }
 if(IdealIndex== NumIdealInits) { ..}
 if(!InitsWithIdealIndex.empty() {
    // emit diag
  }
}
```


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:5337
+
+  // If possible, add fix to the last diagnostic. The fix will reorder all
+  // initializers. This is preferable to attaching overlapping fixes for each
----------------
I totally agree with having a single fix-it to re-order all of the initializers, but I am not sure if last initializer is the best location for that fixit, especially for interactive tools (it is definitely the right behaviour for command line tools like clang and clang-tidy tho, as they'll either print the fix multiple times or try to apply conflicting fixes)

e.g. if user sees 3 out-of-order initializer warnings, they'll likely hover over one of them and say, "aaah" and fix the code themselves, if fixit wasn't available with that one.
Moreover when there's only 1 out-of-order initialzier warning, they'll see the fixit attached no matter what.
Next time, when there are multiple warnings they won't see it and will likely file bugs saying that "clang(d) doesn't generate fixes all the time".

But I also don't have any better suggestions, as having it multiple times will definitely be helpful for interactive tools but will regress the others.. Maybe attaching it to the first initializer might be better overall?


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:5348
+  for (const auto &Init : Inits) {
+    if (Init->getSourceLocation().isMacroID() ||
+        !SrcMgr.isWrittenInSameFile(Init->getSourceLocation(),
----------------
can we have some tests for this case?


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:5355
+  }
+  if (ShouldEmitFix)
+    ShouldEmitFix =
----------------
nit: `ShouldEmitFix = ShouldEmitFix && !Range...`


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:5358
+        !RangeContainsComments(SemaRef, Inits.front()->getSourceLocation(),
+                               Inits.back()->getSourceLocation());
 
----------------
what if there are comments after last initializer ? I think we should rather use LBraceLoc for the constructor in here.


================
Comment at: clang/test/SemaCXX/warn-reorder-ctor-initialization.cpp:139
+    struct Y {
+      template <class T> Y(T x) : a(), X(x) {}
+      Foo a;
----------------
i think it is enough to have `int a;` as a member do we really need a separate type? + why don't we just merge this with the previous case?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87244