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