This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy ObjC] [2/3] Support non-C++ files in ClangTidyTest
ClosedPublic

Authored by benhamilton on Oct 23 2017, 9:46 AM.

Details

Summary

This is part 2 of 3 of a series of changes to improve
Objective-C linting in clang-tidy.

Currently, clang::tidy::test::runCheckOnCode() assumes all files
are C++ and unconditionally adds -std=c++11 to the list of
clang-tidy options.

This updates the logic to check the extension of the source file
and only add -std=c++11 if the extension indicates C++ or
Objective-C++.

Depends On D39188

Test Plan:

ninja ClangTidyTests && \
./tools/clang/tools/extra/unittests/clang-tidy/ClangTidyTests

Diff Detail

Repository
rL LLVM

Event Timeline

benhamilton created this revision.Oct 23 2017, 9:46 AM
hokein accepted this revision.Oct 24 2017, 12:57 AM

LGTM.

unittests/clang-tidy/ClangTidyTest.h
90 ↗(On Diff #119877)

nit: how about

std::string FilenameStr = Filename.str();
auto extension = llvm::sys::path::extension(FilenameStr);

?

This revision is now accepted and ready to land.Oct 24 2017, 12:57 AM
  • @hokein: Tidy up llvm::sys::path::extension call
benhamilton added inline comments.Oct 24 2017, 9:13 AM
unittests/clang-tidy/ClangTidyTest.h
90 ↗(On Diff #119877)

I was trying to avoid a copy:

Twine -> StringRef -> llvm::sys::path::extension()

vs.

Twine -> StringRef -> std::string -> StringRef -> llvm::sys::path::extension()

but I guess readability is more important here. :)

Changed to:

auto extension = llvm::sys::path::extension(Filename.str());
hokein added inline comments.Oct 25 2017, 1:01 AM
unittests/clang-tidy/ClangTidyTest.h
90 ↗(On Diff #119877)

The new change auto extension = llvm::sys::path::extension(Filename.str()); will have use-after-scope problem.

The std::string constructed from Filename.str() will be destructed after this statement, however, extension is a StringRef, which points to the std::string, bad things can happen when accessing the extension variable.

The original version is fine, because we hold the string memory in the SmallVector<char, 10> FilenameChars.

benhamilton marked an inline comment as done.
unittests/clang-tidy/ClangTidyTest.h
90 ↗(On Diff #119877)

Thank you for noticing, fixed.

hokein accepted this revision.Oct 25 2017, 8:30 AM

Still LGTM.

This revision was automatically updated to reflect the committed changes.