This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Add support for custom configuration file names/formats.
ClosedPublic

Authored by alexfh on Oct 16 2014, 5:08 AM.

Details

Summary

We're using different clang-tidy frontends (command-line, batch analysis jobs, code review integration), some of which are limited in the choice of configuration format. In order to avoid duplication of configuration information, we need to support the same configuration format in the command-line tool. This patch adds an extension point to make this possible without rewriting FileOptionsProvider.

Diff Detail

Event Timeline

alexfh updated this revision to Diff 15009.Oct 16 2014, 5:08 AM
alexfh retitled this revision from to [clang-tidy] Add support for custom configuration file names/formats..
alexfh updated this object.
alexfh edited the test plan for this revision. (Show Details)
alexfh added a reviewer: djasper.
alexfh added a subscriber: Unknown Object (MLST).
djasper added inline comments.Oct 16 2014, 5:15 AM
clang-tidy/ClangTidyOptions.h
126

I think the naming here is quite confusing:

  1. ConfigHandler: A single handler
  2. ConfigFileHandlers: Multiple handlers
  3. ConfigHandlers: Member variable containing the current ConfigFileHandlers

I suggest swappin #2 and #3 or renaming ConfigHandler to ConfigFileHandler.

alexfh updated this revision to Diff 15010.Oct 16 2014, 5:26 AM

Renamed FileOptionsProvider::ConfigHandler to ConfigFileHandler.

djasper edited edge metadata.Oct 17 2014, 6:13 AM

Either the patch description or the FileOptionsProvider need a lot more documentation describing the intended usage. This should roughly outline why you might want to parse different configuration files/formats and what you need to implement to do so.

clang-tidy/ClangTidyOptions.cpp
183

Not a strong opinion, but using the concrete type here might help understanding this later.

208–217

I'd move this part of the function into getOptions():
a) The first bit doesn't have to be done for each handler, but just once per directory.
b) We are frequently creating a no_such_file_or_directory error here which we are then explicitly checking later. Using an error code in something that seems like the more frequent use case seems off.

clang-tidy/ClangTidyOptions.h
124

I think a ConfigFileHandler should be a proper class and have (at least) three methods:

  • parseFromFile(StringRef file) - Read configuration from file
  • configurationAsText() - Print configuration in input-like format
  • writeToFile(StringRef) - Save configuration to specific file, ignoring other bits in the file.

Not all of them have to be implemented for all handlers, but that seems like a set of useful functionality.

Also, at least the pair's string needs to be documented.

154

This should document in which order config handlers are tried.

alexfh updated this revision to Diff 15082.Oct 17 2014, 8:17 AM
alexfh edited edge metadata.

Added/updated comments, moved the loop over config handlers inside
TryReadConfigFile.

clang-tidy/ClangTidyOptions.cpp
183

Done.

208–217

These are valid points. I've addressed these using the opposite approach: made TryReadConfigFile try all configuration file handlers.

clang-tidy/ClangTidyOptions.h
124

I don't feel that these methods will be immediately useful. We can add them (or alternatives) once we're have the need to surface relevant functionality (e.g. listing supported configuration formats and choosing the format for -dump-config) in the command-line interface.

For now, I suggest leaving it as a pair. I'm adding a comment describing its content and usage.

154

Done.

alexfh updated this object.Oct 17 2014, 8:32 AM
alexfh removed a subscriber: curdeius.
djasper accepted this revision.Oct 20 2014, 4:47 AM
djasper edited edge metadata.

Looks good.

clang-tidy/ClangTidyOptions.cpp
182–192

indentation

clang-tidy/ClangTidyOptions.h
141

It seems complicated to have to specify all of GlobalOptions, DefaultOptions and OverrideOptions. Consider adding overloads or something.

nit: indentation ;-)

This revision is now accepted and ready to land.Oct 20 2014, 4:47 AM
alexfh added inline comments.Oct 20 2014, 5:39 AM
clang-tidy/ClangTidyOptions.cpp
182–192

Done.

clang-tidy/ClangTidyOptions.h
141

For the use case we have, these all parameters are essential. We can add more overloads once we have a specific need.

alexfh closed this revision.Oct 20 2014, 5:39 AM