This is an archive of the discontinued LLVM Phabricator instance.

Added support of configuration files
ClosedPublic

Authored by sepavloff on Sep 26 2016, 9:56 AM.

Details

Summary

Added support of configuration files

Configuration files store set of options and allow to apply all of them
in a simple way, just by selection of particular file. This change extends
CommandLine Library with functions that read such files.

Diff Detail

Event Timeline

sepavloff updated this revision to Diff 72508.Sep 26 2016, 9:56 AM
sepavloff retitled this revision from to Added support of configuration files.
sepavloff updated this object.
sepavloff added a reviewer: rnk.
sepavloff added a subscriber: llvm-commits.
ABataev added inline comments.
lib/Support/CommandLine.cpp
956

What if Dir is nullptr?

977

What if Argv is empty?

984

'cfg_option' does not follow LLVM coding standard. And 'x'

1000–1001

Use llvm::sys::path::replace_extension(CfgFile, ".cfg")

1036

Remove 'else', unconditional 'return CfgFileSearch::Ignored;'

1143

What if Cur == Source.begin()?

sepavloff marked 2 inline comments as done.Sep 28 2016, 1:37 AM
sepavloff added inline comments.
lib/Support/CommandLine.cpp
956

It would be programmer's error. Added assert.

977

It would be programmers's error - Argv must represents argv from main. Added assert.

984

Fixed.

1000–1001

That would change semantics. A user can use something like testing.64 as a config name, it would be silently converted to testing.cfg which is not the desired behavior.

1036

Fixed.

1143

This is an error. Rewrote this place, added unit tests for this function.

sepavloff updated this revision to Diff 72776.Sep 28 2016, 1:37 AM
sepavloff marked 2 inline comments as done.

Updated patch according to reviewer's notes.

sepavloff updated this revision to Diff 73037.Sep 30 2016, 4:55 AM

Updated patch

Based on feedback on mail list added three function instead of cl::findConfigFile.
It allows a user to make more flexible config file search. Also made some cosmetic
changes.

sepavloff updated this revision to Diff 73065.Sep 30 2016, 9:15 AM

Updated documentation

mgorny added a subscriber: mgorny.Oct 2 2016, 9:34 AM
mgorny added inline comments.
lib/Support/CommandLine.cpp
1047

As I've mentioned on D24933, I think it'd be actually better to have the default dirs stored in LLVM rather than provided by the tools. This way, we can get them consistent in all LLVM tools that are going to use the configuration files, and avoid repeating them in every one of them.

sepavloff updated this revision to Diff 74696.Oct 14 2016, 8:37 AM

Fixed comments and small error in unit test.

ABataev added inline comments.Oct 17 2016, 11:37 AM
include/llvm/Support/CommandLine.h
1780

\brief tag is not required

1789

s/TokenizeConfigFile/tokenizeConfigFile/g

1805

No \brief

1820

No \brief

1831

No \brief

1844

No \brief

1858

No \brief

lib/Support/CommandLine.cpp
710–711

Is this sequence expected on Linux too?

1006

Use llvm::sys::path::extension(CfgFile) != ".cfg" instead

sepavloff updated this revision to Diff 74947.Oct 17 2016, 11:15 PM

Addressed reviewer's notes.

sepavloff marked 8 inline comments as done.Oct 17 2016, 11:20 PM
sepavloff added inline comments.
lib/Support/CommandLine.cpp
710–711

Config file can be prepared on Windows machine and then be used on Linux. Processing Windows EOLs on Linux host saves users from unexpected behavior in this case.

sepavloff updated this revision to Diff 78145.Nov 16 2016, 1:28 AM

Updated patch

Several small changes:

  • Improved documentation,
  • cl::readConfigFile got new argument, number of inserted argument. It may be useful if the option read from config file need to be validated.
  • function cl::findDefaultCfgFile is renamed to cl::searchForFile. In fact it is general purpose function, not config file specific.
sepavloff updated this object.Nov 16 2016, 1:31 AM
sepavloff updated this revision to Diff 78651.Nov 20 2016, 12:20 AM

Updated patch

  • Enhanced documentation,
  • Search for config files specified by --config is allowed in binary directory as well,
  • Does not add ".cfg" to config files automatically.
sepavloff updated this revision to Diff 86760.Feb 1 2017, 7:01 PM

Updated patch

Simplified the patch:

  • removed suport of reading config file name from environment variable.

It is more convenient to read options from such variable rather than the
name of config file. Config file still can be specified by environment
variable using option --config.

  • API function do not return error code anymore, they set string argument

to the error text.

Also the function readConfigFile now returns boolean value to indicate
if config file was successfully read.

sepavloff updated this revision to Diff 98325.May 9 2017, 11:32 AM

Simplified patch

Configuration files are no more considered a feature of CommandLine
Library. Now the patch contains only minimal set of function necessary
for operations on config files. All other functionality is moved to
clang sources.

sepavloff edited the summary of this revision. (Show Details)May 9 2017, 11:34 AM
sepavloff updated this revision to Diff 110227.Aug 8 2017, 10:35 AM

Updated patch

Support and tests for comments and line continuation are removed from
this patch. These features are now presented in D36045. Special
function for reading config files is still required as nested @file
must be resolved relative to including file, not current directory.

Updated patch

Processing comments in config files is now made in the function
tokenizeConfigFile.

Could this patch be reviewed?
It is the only dependency for D24933 (Enable configuration files in clang), which is now accepted.

hfinkel added inline comments.Dec 28 2017, 4:26 PM
lib/Support/CommandLine.cpp
709

This seems like a breaking change. Can you add a parameter to control this so that this only happens when called from cl::tokenizeConfigFile.

Updated patch

Treatment of trailing backslashes is moved to tokenizeConfigFile.
Now all operations related to config file support are separated from
the rest of code.

hfinkel accepted this revision.Dec 29 2017, 7:12 AM

LGTM

This revision is now accepted and ready to land.Dec 29 2017, 7:12 AM
This revision was automatically updated to reflect the committed changes.