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
compatibility reason. The file in `@file` is resolved relative to current
directory in both msvc and libiberty. Current directory may change during
build, so use of `@file` to deliver set of flags to every invocation of clang
becomes unrealistic. Config files instead are taken from well-known places and
if a user set `CFLAGS='--config abc.cfg'` in call to make, it should work as
intended.
> For `clang --config myfile.cfg`, should it also search the current working
> directory?
I believe no, current directory is not reliable place for configurations, as it
may change during build.
> 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
*only* in the directory where `blah-clang` resides. It prevents a user from
overwriting 'system' config files. The idea is that the files in the binary
directory are prepared by SDK suppliers who adapt clang for specific needs.
> I'm not keen on it searching in `~/.llvm`.
The patch is changed so that `~/.llvm` is searched only of config files
specified explicitly by `--config`. A user can specify full path to the config
file: `clang --config ~/.llvm/abc.cfg`. By making the search in `~/.llvm` we
make work with config files a bit more convenient.
> you can replace a blah-clang symlink with a shell script containing `exec
> clang @blah.cfg "@$"`
Due to intermediate shell the environment variables which were not exported
would be lost. This solution is OK for build system but from viewpoint of
compiler, which must work in any build system, it is too fragile.
================
Comment at: docs/UsersManual.rst:667
+extension `cfg` if it is not specified yet, and the obtained file name is
searched
+for in the directories: `~/.llvm` and the directory where clang executable
resides.
+The first found file is used. It is an error if the required file cannot be
found.
----------------
srhines wrote:
> rsmith wrote:
> > hans wrote:
> > > The "add .cfg extension" magic seems a little awkward. It seems this is
> > > mixing the possibility of taking a filename and taking some other name.
> > >
> > > For `clang --config myfile.cfg`, should it also search the current
> > > working directory?
> > >
> > > I'm not keen on it searching in `~/.llvm`.
> > I'm also not keen on searching `~/.llvm`. Whoever makes the `blah-clang`
> > symlink should get to control what the default configuration for
> > `blah-clang` is, I think. But then this seems to immediately lead to the
> > conclusion that we don't need this implicit-config-file feature at all,
> > since you can replace a `blah-clang` symlink with a shell script containing
> > `exec clang @blah.cfg "@$"` -- and it's better to handle it that way, since
> > you get more control over where the config file lives.
> Android is essentially taking the shell script wrapper approach today
> (although we are using python, and we don't do a lot with it just yet).
Agree, removed this feature.
================
Comment at: docs/UsersManual.rst:694
+the including file. For instance if a config file `~/.llvm/target.cfg` contains
+directive `os/linux.opts`, the file `linux.opts` is searched for in the
directory
+`~/.llvm/os`.
----------------
hans wrote:
> I guess you mean directive `@os/linux.opts`?
Yes, thank you for the catch.
================
Comment at: test/Driver/config-file.c:10
+
+// RUN: not %clang --config inexistent.cfg -c %s -### 2>&1 | FileCheck %s
-check-prefix CHECK-INEX-NOSEARCH
+// CHECK-INEX-NOSEARCH: Configuration {{.*}}inexistent.cfg' specified by
option '--config' cannot be found in directories:
----------------
hans wrote:
> This test will fail if I have a `~/.llvm/inexistent.cfg` file on my machine.
Changed the name to more long and awkward to reduce chance of clash.
================
Comment at: test/Driver/config-file.c:13
+// CHECK-INEX-NOSEARCH: ~/.llvm
+// CHECK-INEX-NOSEARCH: /etc/llvm
+
----------------
hans wrote:
> Where did `/etc/llvm` come from?
Remnants of the previous variant, removed.
================
Comment at: test/Driver/config-file.c:14
+// CHECK-INEX-NOSEARCH: /etc/llvm
+
+// RUN: %clang --config %S/Inputs/config-2.cfg -### 2>&1 | FileCheck %s
-check-prefix CHECK-HHH-NOFILE
----------------
hans wrote:
> There is no test checking the functionality that finds a config file based on
> the clang executable name.
Added tests (file config-file2.c).
https://reviews.llvm.org/D24933
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
