astrelni marked 4 inline comments as done.
astrelni added a comment.
In D62977#1560879 <https://reviews.llvm.org/D62977#1560879>, @lebedev.ri wrote:
> Looks reasonable. I did not review the check itself though.
> Are `test/clang-tidy/google-upgrade-googletest-case-nosuite.cpp` and
> `test/clang-tidy/google-upgrade-googletest-case.cpp ` identical other than
> the included header and expected output?
> I'd recommend to condense it into a single file, and just have two `RUN`
> lines each one checking different message prefixes
The nosuite test was a strict subset. I've combined them with a few lines that
needed to be turned via preprocessor branches. Thank you, I actually wasn't
aware that these tests can have multiple `RUN` lines.
================
Comment at: clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.h:34
+private:
+ std::unordered_set<unsigned> MatchedTemplateLocations;
+};
----------------
lebedev.ri wrote:
> Have you tried `llvm::DenseSet` instead?
> This //may// not matter *here*, but `std::unordered_set` usually results in
> horrible perf.
Thanks, I wasn't aware of what is available.
================
Comment at:
clang-tools-extra/test/clang-tidy/google-upgrade-googletest-case-nosuite.cpp:9
+void DummyFixTarget() {}
+// CHECK-FIXES: void DummyFixTarget() {}
+
----------------
lebedev.ri wrote:
> Hm?
I added a comment to better explain this in the combined test.
check_clang_tidy.py asserts that there is at least one message, fix or note
comment present in the file. If there isn't one, the script returns without
even running the test. I couldn't see an option to pass that would turn of this
check, please let me know if you are aware of one.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D62977/new/
https://reviews.llvm.org/D62977
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
