[PATCH] D24933: Enable configuration files in clang

This revision was automatically updated to reflect the committed changes. Closed by commit rL321587: Enable configuration files in clang (authored by sepavloff, committed by ). Repository: rL LLVM https://reviews.llvm.org/D24933 Files: cfe/trunk/docs/UsersManual.rst cfe/trunk/include/clan

[PATCH] D24933: Enable configuration files in clang

sepavloff updated this revision to Diff 127911. sepavloff marked 2 inline comments as done. sepavloff added a comment. Small corrections to the patch - Avoid quick return in the case of error, it may cause fails of clang tools. - Fixed typo. Repository: rC Clang https://reviews.llvm.org/D249

[PATCH] D24933: Enable configuration files in clang

hfinkel accepted this revision. hfinkel added a comment. This revision is now accepted and ready to land. LGTM Comment at: lib/Driver/Driver.cpp:1508 +if (!SystemConfigDir.empty()) + llvm::errs() << "System configuration file directory: " + << SystemC

[PATCH] D24933: Enable configuration files in clang

sepavloff marked 2 inline comments as done. sepavloff added inline comments. Comment at: include/clang/Config/config.h.cmake:40 +#cmakedefine CLANG_CONFIG_FILE_SYSTEM_DIR ${CLANG_CONFIG_FILE_SYSTEM_DIR} +#cmakedefine CLANG_CONFIG_FILE_USER_DIR ${CLANG_CONFIG_FILE_USER_DIR} +

[PATCH] D24933: Enable configuration files in clang

sepavloff updated this revision to Diff 127109. sepavloff added a comment. Updated patch Fixed cmake config file definition. Use llvm::sys::fs::make_absolute to get absolute path. https://reviews.llvm.org/D24933 Files: docs/UsersManual.rst include/clang/Basic/DiagnosticDriverKinds.td inc

[PATCH] D24933: Enable configuration files in clang

hintonda added inline comments. Comment at: include/clang/Config/config.h.cmake:40 +#cmakedefine CLANG_CONFIG_FILE_SYSTEM_DIR ${CLANG_CONFIG_FILE_SYSTEM_DIR} +#cmakedefine CLANG_CONFIG_FILE_USER_DIR ${CLANG_CONFIG_FILE_USER_DIR} + These need to be in quotes since

[PATCH] D24933: Enable configuration files in clang

hfinkel added inline comments. Comment at: lib/Driver/Driver.cpp:641 + +static std::string getAbsolutePath(StringRef P) { + if (P.empty()) Use llvm::sys::fs::make_absolute instead of this. https://reviews.llvm.org/D24933

[PATCH] D24933: Enable configuration files in clang

sepavloff updated this revision to Diff 123503. sepavloff added a comment. Updated patch - Added command line option for setting directories where config files are searched for, - Fixed architecture calculation, - Option --config in config file is now diagnosed, - Output made by option -v contai

[PATCH] D24933: Enable configuration files in clang

sepavloff marked 3 inline comments as done. sepavloff added inline comments. Comment at: lib/Driver/Driver.cpp:739 + // like: i386-clang.cfg -> x86_64-clang.cfg. + if (ArchPrefixLen < CfgFileName.size()) +FixedConfigFile += CfgFileName.substr(ArchPrefixLen); ---

[PATCH] D24933: Enable configuration files in clang

hfinkel added inline comments. Comment at: lib/Driver/Driver.cpp:706 + + // Determine architecture part of the file name, if it presents. + size_t ArchPrefixLen = 0; if it presents. -> if it is present. Comment at: lib/Driver/Driver.cpp:739 +

[PATCH] D24933: Enable configuration files in clang

sepavloff marked 4 inline comments as done. sepavloff added inline comments. Comment at: lib/Driver/Driver.cpp:637 + } + FilePath.clear(); + return false; hfinkel wrote: > Why do we do clear FilePath here? The intent was to avoid copying file name buffer. It h

[PATCH] D24933: Enable configuration files in clang

sepavloff updated this revision to Diff 120403. sepavloff added a comment. Addressed reviewer's notes - Do not process binary directory separately but add it to the array of search directories. - Do not use result file name buffer for temporary data. https://reviews.llvm.org/D24933 Files: d

[PATCH] D24933: Enable configuration files in clang

hfinkel added inline comments. Comment at: lib/Driver/Driver.cpp:661 + + // If not found, try searching the directory where executable resides. + FilePath.clear(); hfinkel wrote: > Why not just add this directory to the end of the list of directories? (by which

[PATCH] D24933: Enable configuration files in clang

hfinkel added inline comments. Comment at: lib/Driver/Driver.cpp:630 + for (const char *Dir : Directories) { +assert(Dir); +FilePath.clear(); assert(Dir && "Directory path should not be null"); Comment at: lib/Driver/Driver.cpp:637 +

[PATCH] D24933: Enable configuration files in clang

sepavloff marked 28 inline comments as done. sepavloff added a comment. @hfinkel Thank you for explanations! https://reviews.llvm.org/D24933 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-c

[PATCH] D24933: Enable configuration files in clang

sepavloff updated this revision to Diff 118315. sepavloff added a comment. Updated patch according to reviewer's notes https://reviews.llvm.org/D24933 Files: docs/UsersManual.rst include/clang/Basic/DiagnosticDriverKinds.td include/clang/Config/config.h.cmake include/clang/Driver/Driver

[PATCH] D24933: Enable configuration files in clang

hfinkel added a comment. Some suggested improvements to the English in the documentation... Comment at: docs/UsersManual.rst:700 + +Configuration files group command line options and allow to specify all of +them just by referencing the configuration file. They may be used, for

[PATCH] D24933: Enable configuration files in clang

sepavloff added a comment. Here is a list of design solutions used in this implementation of config files. **How config file is specified** There are two ways to specify config file: - To encode it into executable file name, such as `foo-clang`, - To pass config file in command line arguments.

[PATCH] D24933: Enable configuration files in clang

sepavloff updated this revision to Diff 110909. sepavloff edited the summary of this revision. sepavloff added a comment. Updated patch - Some functionality, not related to config files directly, is move to separate changes - Cleanup of tests. This change does not implement using construct @fil

Re: [PATCH] D24933: Enable configuration files in clang

2017-08-07 1:46 GMT+07:00 Richard Smith : > On 6 August 2017 at 11:15, Serge Pavlov via cfe-commits < > cfe-commits@lists.llvm.org> wrote: > >> 2017-08-06 6:43 GMT+07:00 Hal Finkel : >> >>> On 07/24/2017 10:18 AM, Serge Pavlov wrote: >>> >>> I am thinking about reducing the patch further to leave

Re: [PATCH] D24933: Enable configuration files in clang

On 6 August 2017 at 11:15, Serge Pavlov via cfe-commits < cfe-commits@lists.llvm.org> wrote: > 2017-08-06 6:43 GMT+07:00 Hal Finkel : > >> On 07/24/2017 10:18 AM, Serge Pavlov wrote: >> >> I am thinking about reducing the patch further to leave only the ability >> to include config file when clang

Re: [PATCH] D24933: Enable configuration files in clang

On 08/06/2017 01:15 PM, Serge Pavlov wrote: 2017-08-06 6:43 GMT+07:00 Hal Finkel >: On 07/24/2017 10:18 AM, Serge Pavlov wrote: I am thinking about reducing the patch further to leave only the ability to include config file when clang is called as `targ

Re: [PATCH] D24933: Enable configuration files in clang

2017-08-06 6:43 GMT+07:00 Hal Finkel : > On 07/24/2017 10:18 AM, Serge Pavlov wrote: > > I am thinking about reducing the patch further to leave only the ability > to include config file when clang is called as `target-clang-drivermode`. > It is still useful for cross compilation tasks because: >

Re: [PATCH] D24933: Enable configuration files in clang

On 07/24/2017 10:18 AM, Serge Pavlov wrote: I am thinking about reducing the patch further to leave only the ability to include config file when clang is called as `target-clang-drivermode`. It is still useful for cross compilation tasks because: - It is a convenient way to switch between sup

Re: [PATCH] D24933: Enable configuration files in clang

I am thinking about reducing the patch further to leave only the ability to include config file when clang is called as `target-clang-drivermode`. It is still useful for cross compilation tasks because: - It is a convenient way to switch between supported targets, - SDK producer can ship compiler w

Re: [PATCH] D24933: Enable configuration files in clang

2017-05-10 3:46 GMT+07:00 Richard Smith : > On 1 March 2017 at 02:50, Serge Pavlov via Phabricator < > revi...@reviews.llvm.org> wrote: > >> >> Format of configuration file is similar to file used in the construct >> `@file`, it is a set of options. Configuration file have advantage over >> this c

Re: [PATCH] D24933: Enable configuration files in clang

On 1 March 2017 at 02:50, Serge Pavlov via Phabricator < revi...@reviews.llvm.org> wrote: > sepavloff added a comment. > > Glad to know that someone is interested in this feature! > Below is actual proposal. > > **Adding named configuration files to clang driver** > > A configuration file is a col

[PATCH] D24933: Enable configuration files in clang

sepavloff updated this revision to Diff 98330. sepavloff added a comment. Herald added subscribers: krytarowski, rengolin. Updated patch https://reviews.llvm.org/D24933 Files: docs/UsersManual.rst include/clang/Basic/DiagnosticDriverKinds.td include/clang/Config/config.h.cmake include/c

[PATCH] D24933: Enable configuration files in clang

sepavloff added a comment. Glad to know that someone is interested in this feature! Below is actual proposal. **Adding named configuration files to clang driver** A configuration file is a collection of driver options, which are inserted into command line before other options specified in the c

[PATCH] D24933: Enable configuration files in clang

dschuff added a comment. Hello, I work on WebAssembly toolchains (including emscripten, which is more or less the "cross-compiler SDK" use case). There's a lot of history in this review and I haven't looked at the details yet, but does the current summary text reflect the current proposal and/o

[PATCH] D24933: Enable configuration files in clang

sepavloff marked 2 inline comments as done. sepavloff added inline comments. Comment at: lib/Driver/ToolChain.cpp:183 std::string Target; - if (llvm::TargetRegistry::lookupTarget(Prefix, IgnoredError)) { + if (!VerifyTarget || llvm::TargetRegistry::lookupTarget(Prefix, Igno

[PATCH] D24933: Enable configuration files in clang

sepavloff updated this revision to Diff 87229. sepavloff added a comment. Herald added a subscriber: klimek. Updated patch https://reviews.llvm.org/D24933 Files: docs/UsersManual.rst include/clang/Basic/DiagnosticDriverKinds.td include/clang/Config/config.h.cmake include/clang/Driver/Dr

[PATCH] D24933: Enable configuration files in clang

hfinkel added inline comments. Comment at: include/clang/Driver/Driver.h:219 + /// This number may be smaller that \c NumConfigOptions if some options + /// requires separate arguments. + unsigned NumConfigArgs; requires -> require Comment a

[PATCH] D24933: Enable configuration files in clang

sepavloff updated this revision to Diff 86761. sepavloff added a comment. Herald added a subscriber: danalbert. Updated patch Use more robust algorithm to determine custom compiler prefix. Bring the code in sync with changes in llvm patch. https://reviews.llvm.org/D24933 Files: docs/UsersMan

[PATCH] D24933: Enable configuration files in clang

hfinkel added inline comments. Comment at: lib/Driver/Driver.cpp:172 +NumConfigArgs = static_cast(NumCfgArgs); + } + sepavloff wrote: > bruno wrote: > > If `NumCfgArgs == 0` it would be nice to warn that the config file is empty? > Not sure if it makes sense

[PATCH] D24933: Enable configuration files in clang

sepavloff added inline comments. Comment at: include/clang/Driver/Driver.h:287 + const std::string &getConfigFile() const { return ConfigFile; } + void setConfigFile(StringRef x, unsigned N) { +ConfigFile = x; bruno wrote: > x -> FileName Fixed. =

[PATCH] D24933: Enable configuration files in clang

sepavloff updated this revision to Diff 81227. sepavloff added a comment. Addressed review notes. https://reviews.llvm.org/D24933 Files: docs/UsersManual.rst include/clang/Basic/DiagnosticDriverKinds.td include/clang/Config/config.h.cmake include/clang/Driver/Driver.h lib/Driver/Drive

[PATCH] D24933: Enable configuration files in clang

bruno added inline comments. Comment at: include/clang/Driver/Driver.h:287 + const std::string &getConfigFile() const { return ConfigFile; } + void setConfigFile(StringRef x, unsigned N) { +ConfigFile = x; x -> FileName Comment at: lib/Dr

[PATCH] D24933: Enable configuration files in clang

sepavloff updated this revision to Diff 81057. sepavloff added a comment. Fixed grammar in comment, thanks to Hal Finkel. https://reviews.llvm.org/D24933 Files: docs/UsersManual.rst include/clang/Basic/DiagnosticDriverKinds.td include/clang/Config/config.h.cmake include/clang/Driver/Dri

[PATCH] D24933: Enable configuration files in clang

hfinkel added inline comments. Comment at: lib/Driver/Driver.cpp:2695 + // Claim all arguments that come from configuration file, - driver must not + // warn about unused argument on them. Grammar here is a bit odd, how about: // Claim all arguments that c

[PATCH] D24933: Enable configuration files in clang

sepavloff updated this revision to Diff 81055. sepavloff marked 4 inline comments as done. sepavloff added a comment. Updated patch. - Directories which are searched for config files are now defined during project configuration process using special cmake options. - Driver do not warn on unused

[PATCH] D24933: Enable configuration files in clang

mgorny added a comment. In https://reviews.llvm.org/D24933#600884, @sepavloff wrote: > > Whoever makes the `blah-clang` symlink should get to control what the > > default configuration for `blah-clang` is, I think. > > The patch is changed so that config file for `blah-clang` is searched for >

[PATCH] D24933: Enable configuration files in clang

sepavloff marked 5 inline comments as done. sepavloff added a comment. > Rather than inventing a new mechanism, could we extend our existing `@file` > facility to support comments and nested inclusion of further `@file`s? Reusing `@file` is an attractive idea, but it cannot be implemented due to

[PATCH] D24933: Enable configuration files in clang

hfinkel added a comment. What happens with unused arguments in the configuration files? This feature looks potentially useful for me, but only if it suppresses unused-argument warnings. For example, if I put -L/usr/local/lib -stdlib=libc++ into a configuration file, I can't have these: cl

[PATCH] D24933: Enable configuration files in clang

sepavloff updated this revision to Diff 78652. sepavloff added a comment. Herald added a subscriber: aemerson. Updated patch. - Restrict the search for config files specified by mangled clang name to binary directory only, - Allow the search for config files specified by `--config` in binary dir

[PATCH] D24933: Enable configuration files in clang

srhines added a comment. Thanks to Hans for notifying me about this. Cross-compilation flags are a big challenge in Android, but I have similar concerns about looking for magic files/dirs. Richard's suggestion about improving @file is really interesting, and would satisfy a large part of our ne

[PATCH] D24933: Enable configuration files in clang

rsmith added inline comments. Comment at: docs/UsersManual.rst:655-656 + +Command line option `--config` can be used to specify configuration file in +a clang invocation. For instance: + Rather than inventing a new mechanism, could we extend our existing `@file`

[PATCH] D24933: Enable configuration files in clang

hans added a subscriber: srhines. hans added a comment. I'm still skeptical, but I think this is an improvement over the previous patch. I think your best bet to get this landed is to find someone from the cfe-dev thread who is in favour of config files and have them review it. I'm also cc'ing

[PATCH] D24933: Enable configuration files in clang

sepavloff updated this revision to Diff 78167. sepavloff added a comment. Limit the patch by named configuration files only Follow advice of @mgorny and removed all features from the patch except named config files. These are files specified explicitly by option `--config` or encoded in executabl

[PATCH] D24933: Enable configuration files in clang

mgorny added a comment. @sepavloff, my suggestion would be to rework the patch to the minimal, acceptable feature (i.e. executable-relative config), and then possibly send more patches for the additional features that require additional discussion. This way, we'll at least get something done in

[PATCH] D24933: Enable configuration files in clang

sepavloff added a comment. >> Config file set by environment variable is similar to default config file as >> user does not specify anything in command line. But the user must explicitly >> specify the file, it is not made automatically. > > Sorry, I don't understand this part. If a user speci

[PATCH] D24933: Enable configuration files in clang

hans added a comment. In https://reviews.llvm.org/D24933#591943, @sepavloff wrote: > >> Default config file is searched for only in the directory where clang > >> executable resides. It allows SDK suppliers to customize compiler, for > >> instance by turning off unneeded warnings, by providing

[PATCH] D24933: Enable configuration files in clang

sepavloff added a comment. >> Default config file is searched for only in the directory where clang >> executable resides. It allows SDK suppliers to customize compiler, for >> instance by turning off unneeded warnings, by providing their own clang.cfg. >> On the other hand developers cannot by

[PATCH] D24933: Enable configuration files in clang

hans added a comment. In https://reviews.llvm.org/D24933#590493, @sepavloff wrote: > > For Chromium, our build system provides a specific Clang version close to > > ToT, and obviously what flags to use when invoking it. (Developers can of > > course override the flags when configuring if they w

[PATCH] D24933: Enable configuration files in clang

sepavloff added a comment. > For Chromium, our build system provides a specific Clang version close to > ToT, and obviously what flags to use when invoking it. (Developers can of > course override the flags when configuring if they want.) Now maybe a > developer has a ~/clang.cfg because they w

[PATCH] D24933: Enable configuration files in clang

mgorny added a comment. I don't agree with the argumentation of @hans; however, I have my own concerns. I personally dislike the idea of reusing command-line option format for this. While I can see this is the simplest solution, it brings at least a few problems and questions. I'm afraid that t

[PATCH] D24933: Enable configuration files in clang

hans added a comment. Apologies for seeming so negative here. I can see that this would possibly be beneficial for some users. This would be a powerful feature, which is what worries me. Here's a specific scenario I'm worried about. For Chromium, our build system provides a specific Clang vers

[PATCH] D24933: Enable configuration files in clang

sepavloff added a comment. Summary of proposal and discussion in mail list. **Problems** The two main problems this feature addresses are: 1. Ability to modify default compiler settings. As an example, the warning `-Wundefined-var-template` can be helpful for people doing module-enabled build

[PATCH] D24933: Enable configuration files in clang

hans added a comment. I didn't follow the original thread too closely as I didn't really see the problem this was trying to address, but since I'm now cc'd I'll add my opinion. I'm not in favour of this functionality. (But I guess I might be missing the main use case.) The way I see it, what f

[PATCH] D24933: Enable configuration files in clang

rnk added a comment. Added some people who might have opinions on the command line interface. Sorry, I don't have a lot of time for this right now. https://reviews.llvm.org/D24933 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists

[PATCH] D24933: Enable configuration files in clang

mgorny resigned from this revision. mgorny removed a reviewer: mgorny. mgorny added a comment. I have no further comments. However, I'm quite a fresh contributor, so I'm not really in a position to accept it. https://reviews.llvm.org/D24933 ___ cfe

[PATCH] D24933: Enable configuration files in clang

sepavloff added inline comments. > mgorny wrote in driver.cpp:314 > Please document what this function does, exactly. I see you've documented it > in call site but a doc here would be helpful as well. The function is documented now. > mgorny wrote in driver.cpp:376 > Are you sure about the nam

[PATCH] D24933: Enable configuration files in clang

sepavloff updated this revision to Diff 73482. sepavloff added a comment. Updated comments. https://reviews.llvm.org/D24933 Files: include/clang/Driver/Driver.h lib/Driver/Driver.cpp test/Driver/Inputs/config-1.cfg test/Driver/Inputs/config-2.cfg test/Driver/config-file.c test/Drive

[PATCH] D24933: Enable configuration files in clang

mgorny added inline comments. > driver.cpp:314 > + > +static llvm::cl::SearchResult findConfigFileFromProgramName( > +llvm::SmallVectorImpl &ConfigName, StringRef ProgramName) { Please document what this function does, exactly. I see you've documented it in call site but a doc here would be

[PATCH] D24933: Enable configuration files in clang

sepavloff added inline comments. > mgorny wrote in driver.cpp:334 > 1. I'm not sure if others would agree with me but I think it would be better > to move those default paths straight to LLVM. If others tools are to adopt > those configuration files, it would only be reasonable to use the same

[PATCH] D24933: Enable configuration files in clang

sepavloff updated this revision to Diff 73245. sepavloff added a comment. Updated path The patch is aligned with corresponding changes in llvm repository (https://reviews.llvm.org/D24926). Also support of configuration selection for executables line foo-clang is added. https://reviews.llvm.org

[PATCH] D24933: Enable configuration files in clang

mgorny requested changes to this revision. mgorny added a reviewer: mgorny. mgorny added a comment. This revision now requires changes to proceed. Few minor nits. However, I'd like to say that I like the idea in general and see forward to deploy it in Gentoo. One use case which doesn't seem to h

Re: [PATCH] D24933: Enable configuration files in clang

ddunbar added a comment. I am too out of the loop on Clang development to be able to comment on the specific direction, but I will just say that I am highly in favor of adding new features in this direction. Thank you! https://reviews.llvm.org/D24933

[PATCH] D24933: Enable configuration files in clang

sepavloff created this revision. sepavloff added reviewers: rnk, ddunbar. sepavloff added a subscriber: cfe-commits. The implementation follows the discussion in mail list (http://lists.llvm.org/pipermail/cfe-dev/2016-September/050776.html). Main part of the config file support is implemented in l