Page MenuHomePhabricator

Add ExtraArgs and ExtraArgsBefore options to enable clang warnings via configuration files.
ClosedPublic

Authored by alexfh on Oct 29 2015, 5:58 PM.

Diff Detail

Event Timeline

alexfh updated this revision to Diff 38777.Oct 29 2015, 5:58 PM
alexfh retitled this revision from to Add ExtraArgs and ExtraArgsBefore options to enable clang warnings via configuration files..
alexfh updated this object.
alexfh added a reviewer: klimek.
alexfh added a subscriber: cfe-commits.
djasper added a subscriber: djasper.Nov 4 2015, 5:24 PM
djasper added inline comments.
clang-tidy/ClangTidy.cpp
47

This doesn't seem to be the right place for this #include.

clang-tidy/ClangTidyOptions.h
89–93

Are we sure we are going to need both? Maybe for now only add the one you are actually using (and testing)?

alexfh updated this revision to Diff 39376.Nov 5 2015, 9:49 AM
alexfh marked an inline comment as done.

Addressed review comments.

clang-tidy/ClangTidyOptions.h
89–93

Though it's a less frequently used option, but seems to be good for consistency (with the clang tools' -extra-arg/-extra-arg-before pair of options) and for completeness. I've extended the test to verify both options and their interaction.

alexfh removed a subscriber: djasper.

PTAL

djasper edited edge metadata.Nov 5 2015, 4:17 PM

I'll let Manuel review this tomorrow. He expressed some concerns.

klimek added inline comments.Nov 6 2015, 1:49 AM
clang-tidy/ClangTidy.cpp
382

My concern is still with the Filename in the arg callback. I'd like to understand the full use-case better.
From this code it looks like we want to change the ClangTidyContext's file and use the callback for that? That seems rather out of place, but maybe I'm missing something.

alexfh added inline comments.Nov 6 2015, 7:18 AM
clang-tidy/ClangTidy.cpp
382

We need the filename to retrieve the proper ClangTidyOptions for each file. Context.setCurrentFile is used to specify which file should getOptions return the options for.

klimek added inline comments.Nov 6 2015, 7:25 AM
clang-tidy/ClangTidy.cpp
382

But the extra-args don't depend on the file; it seems like the filename in the callback is only used to be notified when the file changes?

alexfh added inline comments.Nov 6 2015, 7:29 AM
clang-tidy/ClangTidy.cpp
382

The thing is that they do depend on the file, because we're implementing their configuration from the config file, and different translation units can have different corresponding configuration files.

klimek added inline comments.Nov 6 2015, 7:32 AM
clang-tidy/ClangTidy.cpp
382

Ok, now I get it. I completely didn't expect an interface that needs a setCurrentFile so it can getOptions afterwards for that file.
In that case, the Filename is OK, but I suggest changing the ClangTidyOptions to provide getOptions(file) instead.

alexfh updated this revision to Diff 39551.Nov 6 2015, 9:52 AM
alexfh edited edge metadata.

Added ClangTidyContext::getOptionsForFile.

alexfh updated this revision to Diff 39553.Nov 6 2015, 9:58 AM

Updated documentation comments.

PTAL

clang-tidy/ClangTidy.cpp
382

The interface should be clearer, if you think about the possibility of having different options for different files. Also, the documentation on the corresponding methods should help. But adding getOptionsForFile doesn't hurt.

klimek accepted this revision.Nov 9 2015, 4:42 AM
klimek edited edge metadata.

Ok, this patch LG. I'd really like to get rid of the current-file-dependent stuff in ClangTidyOptions though :)

This revision is now accepted and ready to land.Nov 9 2015, 4:42 AM
alexfh closed this revision.Nov 9 2015, 8:30 AM