hwright marked 4 inline comments as done. hwright added inline comments.
================ Comment at: clang-tidy/abseil/DurationDivisionCheck.h:23 // http://clang.llvm.org/extra/clang-tidy/checks/abseil-duration-division.html class DurationDivisionCheck : public ClangTidyCheck { ---------------- JonasToth wrote: > hwright wrote: > > JonasToth wrote: > > > I think that blank line could be removed, and it seems the comment is not > > > ///, could you take a look at it too? > > > Touching this file is probably better to do in another patch anyway. > > Agreed. I think this snuck into the patch; I'll remove it. > > > > (It would be good to just `clang-format` everything in this directory in a > > separate patch.) > > > > The comment issue with `///` seems to be a common problem; is > > `clang-tidy/add_new_check.py` not generating correct code? > add_new_check does ///, maybe IDE settings or so removed these? Maybe someone > created everything manually, dunno. > > Doing the clang-format is ok, doesn't need review either (but plz run the > test before committing to master). I don't think I yet have the commit bit...so somebody else wouldn't need to directly commit. :) ================ Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:61 + + if (SimpleArg) { diag(MatchedCall->getBeginLoc(), ---------------- JonasToth wrote: > hwright wrote: > > JonasToth wrote: > > > The diagnostic is not printed if for some reason the fixit was not > > > creatable. I think that the warning could still be emitted (`auto Diag = > > > diag(...); if (Fix) Diag << Fixit::-...`) > > I'm not sure under which conditions you'd expect this to be an issue. > > Could you give me an example? > > > > My expectation is that if we don't get a value back in `SimpleArg`, we > > don't have anything to change, so there wouldn't be a warning to emit. > I don't expect this to fail, failure there would mean a bug i guess. Having > bugs is somewhat expected :) > And that would be our way to find the bug, because some user reports that > there is no transformation done, that is my motivation behind that. > > The warning itself should be correct, otherwise the matcher does not work, > right? This would just be precaution. I guess what I'm saying is that I'm not sure what kind of warning we'd give if we weren't able to offer a fix. The optionality here means that we found a result which was already as simple as we can make it, so there's no reason to bother the user. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54737/new/ https://reviews.llvm.org/D54737 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
