[clang] RFC: [cmake] Export CLANG_RESOURCE_DIR in ClangConfig (PR #97197)

[Kim Gräsman via cfe-commits]

kimgr wrote:

https://github.com/llvm/llvm-project/pull/103388 was merged, I think I know how 
to work this now. Thanks everyone for helping!

https://github.com/llvm/llvm-project/pull/97197
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] RFC: [cmake] Export CLANG_RESOURCE_DIR in ClangConfig (PR #97197)

[Kim Gräsman via cfe-commits]

https://github.com/kimgr closed https://github.com/llvm/llvm-project/pull/97197
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [lldb] Use CLANG_RESOURCE_DIR more consistently (PR #103388)

[Kim Gräsman via cfe-commits]

kimgr wrote:

Thanks!

https://github.com/llvm/llvm-project/pull/103388
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [lldb] Use CLANG_RESOURCE_DIR more consistently (PR #103388)

[Kim Gräsman via cfe-commits]

kimgr wrote:

> > Do we need anything more to make progress with this PR?
> 
> @kimgr Do you have committer permission? Would you like some help to get this 
> merged?

Oh, no, I don't, I would need someone to merge this for me. It's still pretty 
early in the v.20 cycle, right, so maybe this is a good time to try and see if 
it sticks?

https://github.com/llvm/llvm-project/pull/103388
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [lldb] Use CLANG_RESOURCE_DIR more consistently (PR #103388)

[Kim Gräsman via cfe-commits]

kimgr wrote:

> > Is there an out-of-tree scenario where CLANG_RESOURCE_DIR needs to be 
> > replaced with something else at runtime, i.e. a real use-case for the 
> > optional CustomResourceDir optional argument I removed?
> 
> @kimgr I have also looked for this and I haven't found an use-case where this 
> would be appropriate. In fact, I think it's wrong to not take 
> `CLANG_RESOURCE_DIR` as part of the output from 
> `driver::Driver::GetResourcesPath()` because that would generate wrong 
> results for builds that do set `CLANG_RESOURCE_DIR`, e.g. Fedora's builds.

@tuliom I think it might be possible that a standalone tool would want to use 
its own executable path and a hardcoded relative `CustomResourceDir`, but then 
maybe it would be better to just build the relevant path without calling 
`GetResourcesPath`.

https://github.com/llvm/llvm-project/pull/103388
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [lldb] Use CLANG_RESOURCE_DIR more consistently (PR #103388)

[Kim Gräsman via cfe-commits]

kimgr wrote:

Do we need anything more to make progress with this PR?

My own primary concerns are basically things I can't check myself:

* Is there an out-of-tree scenario where `CLANG_RESOURCE_DIR` needs to be 
replaced with something else at runtime, i.e. a real use-case for the optional 
`CustomResourceDir` optional argument I removed?
* The only call in-tree whose behavior changed is 
https://github.com/llvm/llvm-project/blob/99b85cae628c1cc5641944290712cd84ccf1f6c8/clang/tools/libclang/CIndexer.cpp#L156.
 Is there any reason to think libclang should behave differently?

Not sure who can shed light on these questions.

https://github.com/llvm/llvm-project/pull/103388
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [lldb] Use CLANG_RESOURCE_DIR more consistently (PR #103388)

[Kim Gräsman via cfe-commits]

kimgr wrote:

@nico You're the original author of this thing back in 2019, maybe you have 
some additional information. Please take a look if you have a chance.

https://github.com/llvm/llvm-project/pull/103388
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [lldb] Use CLANG_RESOURCE_DIR more consistently (PR #103388)

[Kim Gräsman via cfe-commits]

https://github.com/kimgr updated 
https://github.com/llvm/llvm-project/pull/103388

From 3722d673ee409c1320c9d66aa2f3f6568ffdfd77 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Kim=20Gr=C3=A4sman?= 
Date: Mon, 5 Aug 2024 11:49:32 +0200
Subject: [PATCH] Use CLANG_RESOURCE_DIR more consistently

When Clang is consumed as a library, the CLANG_RESOURCE_DIR definition
is not exported from the CMake system, so external clients will be
unable to compute the same resource dir as Clang itself would, because
they don't know what to pass for the optional CustomResourceDir
argument.

All call sites except one would pass CLANG_RESOURCE_DIR to
Driver::GetResourcesPath. It seems the one exception in libclang
CIndexer was an oversight.

Move the use of CLANG_RESOURCE_DIR into GetResourcesPath and remove the
optional argument to avoid this inconsistency between internal and
external clients.
---
 clang/include/clang/Driver/Driver.h|  3 +--
 clang/lib/Driver/Driver.cpp| 14 +++---
 clang/lib/Frontend/CompilerInvocation.cpp  |  2 +-
 .../Plugins/ExpressionParser/Clang/ClangHost.cpp   |  2 +-
 lldb/unittests/Expression/ClangParserTest.cpp  |  4 ++--
 5 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/clang/include/clang/Driver/Driver.h 
b/clang/include/clang/Driver/Driver.h
index 04b46782467d6a..0382c8cb155e51 100644
--- a/clang/include/clang/Driver/Driver.h
+++ b/clang/include/clang/Driver/Driver.h
@@ -379,8 +379,7 @@ class Driver {
 
   /// Takes the path to a binary that's either in bin/ or lib/ and returns
   /// the path to clang's resource directory.
-  static std::string GetResourcesPath(StringRef BinaryPath,
-  StringRef CustomResourceDir = "");
+  static std::string GetResourcesPath(StringRef BinaryPath);
 
   Driver(StringRef ClangExecutable, StringRef TargetTriple,
  DiagnosticsEngine &Diags, std::string Title = "clang LLVM compiler",
diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp
index 8e44d5afa40e05..dac8185693cf58 100644
--- a/clang/lib/Driver/Driver.cpp
+++ b/clang/lib/Driver/Driver.cpp
@@ -171,18 +171,18 @@ getHIPOffloadTargetTriple(const Driver &D, const ArgList 
&Args) {
 }
 
 // static
-std::string Driver::GetResourcesPath(StringRef BinaryPath,
- StringRef CustomResourceDir) {
+std::string Driver::GetResourcesPath(StringRef BinaryPath) {
   // Since the resource directory is embedded in the module hash, it's 
important
   // that all places that need it call this function, so that they get the
   // exact same string ("a/../b/" and "b/" get different hashes, for example).
 
   // Dir is bin/ or lib/, depending on where BinaryPath is.
-  std::string Dir = std::string(llvm::sys::path::parent_path(BinaryPath));
-
+  StringRef Dir = llvm::sys::path::parent_path(BinaryPath);
   SmallString<128> P(Dir);
-  if (CustomResourceDir != "") {
-llvm::sys::path::append(P, CustomResourceDir);
+
+  StringRef ConfiguredResourceDir(CLANG_RESOURCE_DIR);
+  if (!ConfiguredResourceDir.empty()) {
+llvm::sys::path::append(P, ConfiguredResourceDir);
   } else {
 // On Windows, libclang.dll is in bin/.
 // On non-Windows, libclang.so/.dylib is in lib/.
@@ -239,7 +239,7 @@ Driver::Driver(StringRef ClangExecutable, StringRef 
TargetTriple,
 #endif
 
   // Compute the path to the resource directory.
-  ResourceDir = GetResourcesPath(ClangExecutable, CLANG_RESOURCE_DIR);
+  ResourceDir = GetResourcesPath(ClangExecutable);
 }
 
 void Driver::setDriverMode(StringRef Value) {
diff --git a/clang/lib/Frontend/CompilerInvocation.cpp 
b/clang/lib/Frontend/CompilerInvocation.cpp
index f6b6c44a4cab6a..6962cb4f32b06a 100644
--- a/clang/lib/Frontend/CompilerInvocation.cpp
+++ b/clang/lib/Frontend/CompilerInvocation.cpp
@@ -3121,7 +3121,7 @@ std::string CompilerInvocation::GetResourcesPath(const 
char *Argv0,
  void *MainAddr) {
   std::string ClangExecutable =
   llvm::sys::fs::getMainExecutable(Argv0, MainAddr);
-  return Driver::GetResourcesPath(ClangExecutable, CLANG_RESOURCE_DIR);
+  return Driver::GetResourcesPath(ClangExecutable);
 }
 
 static void GenerateHeaderSearchArgs(const HeaderSearchOptions &Opts,
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp 
b/lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp
index 6064c02c7fd67d..6de851081598fd 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp
@@ -53,7 +53,7 @@ static bool DefaultComputeClangResourceDirectory(FileSpec 
&lldb_shlib_spec,
   std::string raw_path = lldb_shlib_spec.GetPath();
   llvm::StringRef parent_dir = llvm::sys::path::parent_path(raw_path);
   static const std::string clang_resource_path =
-  clang::driver::Driver::GetResourcesPath("bin/lldb", CLANG_RESOURCE_DIR);
+  clang::driver::Driver::GetResourcesPath("bin/lldb");
 
 

[clang] RFC: [cmake] Export CLANG_RESOURCE_DIR in ClangConfig (PR #97197)

[Kim Gräsman via cfe-commits]

kimgr wrote:

@tstellar 
> I think that the GetResourcesPath function is incorrect. There's no reason it 
> should return a wrong value, since the CLANG_RESOURCE_DIR value is always 
> known at compile time.

Please see https://github.com/llvm/llvm-project/pull/103388 for an attempt to 
make this internally consistent.

https://github.com/llvm/llvm-project/pull/97197
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [lldb] Use CLANG_RESOURCE_DIR more consistently (PR #103388)

[Kim Gräsman via cfe-commits]

https://github.com/kimgr created 
https://github.com/llvm/llvm-project/pull/103388

When Clang is consumed as a library, the CLANG_RESOURCE_DIR definition is not 
exported from the CMake system, so external clients will be unable to compute 
the same resource dir as Clang itself would, because they don't know what to 
pass for the optional CustomResourceDir argument.

All call sites except one would pass CLANG_RESOURCE_DIR to 
Driver::GetResourcesPath. It seems the one exception in libclang CIndexer was 
an oversight.

Move the use of CLANG_RESOURCE_DIR into GetResourcesPath and remove the 
optional argument to avoid this inconsistency between internal and external 
clients.

From 102517cd9d53695c9ae56135492bab09df9d90ee Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Kim=20Gr=C3=A4sman?= 
Date: Mon, 5 Aug 2024 11:49:32 +0200
Subject: [PATCH] Use CLANG_RESOURCE_DIR more consistently

When Clang is consumed as a library, the CLANG_RESOURCE_DIR definition
is not exported from the CMake system, so external clients will be
unable to compute the same resource dir as Clang itself would, because
they don't know what to pass for the optional CustomResourceDir
argument.

All call sites except one would pass CLANG_RESOURCE_DIR to
Driver::GetResourcesPath. It seems the one exception in libclang
CIndexer was an oversight.

Move the use of CLANG_RESOURCE_DIR into GetResourcesPath and remove the
optional argument to avoid this inconsistency between internal and
external clients.
---
 clang/include/clang/Driver/Driver.h|  3 +--
 clang/lib/Driver/Driver.cpp| 14 +++---
 clang/lib/Frontend/CompilerInvocation.cpp  |  2 +-
 .../Plugins/ExpressionParser/Clang/ClangHost.cpp   |  2 +-
 lldb/unittests/Expression/ClangParserTest.cpp  |  2 +-
 5 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/clang/include/clang/Driver/Driver.h 
b/clang/include/clang/Driver/Driver.h
index 04b46782467d6a..0382c8cb155e51 100644
--- a/clang/include/clang/Driver/Driver.h
+++ b/clang/include/clang/Driver/Driver.h
@@ -379,8 +379,7 @@ class Driver {
 
   /// Takes the path to a binary that's either in bin/ or lib/ and returns
   /// the path to clang's resource directory.
-  static std::string GetResourcesPath(StringRef BinaryPath,
-  StringRef CustomResourceDir = "");
+  static std::string GetResourcesPath(StringRef BinaryPath);
 
   Driver(StringRef ClangExecutable, StringRef TargetTriple,
  DiagnosticsEngine &Diags, std::string Title = "clang LLVM compiler",
diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp
index 8e44d5afa40e05..dac8185693cf58 100644
--- a/clang/lib/Driver/Driver.cpp
+++ b/clang/lib/Driver/Driver.cpp
@@ -171,18 +171,18 @@ getHIPOffloadTargetTriple(const Driver &D, const ArgList 
&Args) {
 }
 
 // static
-std::string Driver::GetResourcesPath(StringRef BinaryPath,
- StringRef CustomResourceDir) {
+std::string Driver::GetResourcesPath(StringRef BinaryPath) {
   // Since the resource directory is embedded in the module hash, it's 
important
   // that all places that need it call this function, so that they get the
   // exact same string ("a/../b/" and "b/" get different hashes, for example).
 
   // Dir is bin/ or lib/, depending on where BinaryPath is.
-  std::string Dir = std::string(llvm::sys::path::parent_path(BinaryPath));
-
+  StringRef Dir = llvm::sys::path::parent_path(BinaryPath);
   SmallString<128> P(Dir);
-  if (CustomResourceDir != "") {
-llvm::sys::path::append(P, CustomResourceDir);
+
+  StringRef ConfiguredResourceDir(CLANG_RESOURCE_DIR);
+  if (!ConfiguredResourceDir.empty()) {
+llvm::sys::path::append(P, ConfiguredResourceDir);
   } else {
 // On Windows, libclang.dll is in bin/.
 // On non-Windows, libclang.so/.dylib is in lib/.
@@ -239,7 +239,7 @@ Driver::Driver(StringRef ClangExecutable, StringRef 
TargetTriple,
 #endif
 
   // Compute the path to the resource directory.
-  ResourceDir = GetResourcesPath(ClangExecutable, CLANG_RESOURCE_DIR);
+  ResourceDir = GetResourcesPath(ClangExecutable);
 }
 
 void Driver::setDriverMode(StringRef Value) {
diff --git a/clang/lib/Frontend/CompilerInvocation.cpp 
b/clang/lib/Frontend/CompilerInvocation.cpp
index f6b6c44a4cab6a..6962cb4f32b06a 100644
--- a/clang/lib/Frontend/CompilerInvocation.cpp
+++ b/clang/lib/Frontend/CompilerInvocation.cpp
@@ -3121,7 +3121,7 @@ std::string CompilerInvocation::GetResourcesPath(const 
char *Argv0,
  void *MainAddr) {
   std::string ClangExecutable =
   llvm::sys::fs::getMainExecutable(Argv0, MainAddr);
-  return Driver::GetResourcesPath(ClangExecutable, CLANG_RESOURCE_DIR);
+  return Driver::GetResourcesPath(ClangExecutable);
 }
 
 static void GenerateHeaderSearchArgs(const HeaderSearchOptions &Opts,
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp 
b/lldb/source/Plugins/ExpressionParser/Clan

[clang] RFC: [cmake] Export CLANG_RESOURCE_DIR in ClangConfig (PR #97197)

[Kim Gräsman via cfe-commits]

kimgr wrote:

> The exported target is new and hasn't been in a release yet. It should be in 
> LLVM-19 once it's released and Debian gets updated. Of course, this is also 
> true of any other new thing that gets added now anyway.

👍 

> I am a little confused about what your goal is with the path manipulations 
> though

Aren't we all 🙂? I think what I'm trying to achieve is a portable way to refer 
to or install the built-in headers to avoid spurious " not found" 
errors. My current take is we can defer to clang packaging to put the headers 
on disk, and make them available to IWYU with a package dependency + some kind 
of header search path. I don't think we need anything else from the resource 
dir.


https://github.com/llvm/llvm-project/pull/97197
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] RFC: [cmake] Export CLANG_RESOURCE_DIR in ClangConfig (PR #97197)

[Kim Gräsman via cfe-commits]

kimgr wrote:

@llvm-beanz Thanks for clarifying! Yeah, I no longer think this PR has anything 
to offer. If you think it's future-proof to assume the interface include dir of 
clang-resource-headers is one level down from the resource dir, that works to 
polish and seed it into a hardwired `-resource-dir` switch.

https://github.com/llvm/llvm-project/pull/97197
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] RFC: [cmake] Export CLANG_RESOURCE_DIR in ClangConfig (PR #97197)

[Kim Gräsman via cfe-commits]

kimgr wrote:

@llvm-beanz 

> @etcwilde's point above is that the CLANG_RESOURCE_DIR is already available 
> in CMake for projects
> that import the Clang package or are built in-tree with Clang. 

I don't think so. As far as I can tell:

* `clang-resource-headers` interface header dir would be the full path to the 
builtin headers dir, e.g. `/usr/lib/llvm-19/lib/clang/19/include`
* Unfortunately the Debian packages from apt.llvm.org don't define 
`clang-resource-headers`, though the target name is listed in 
`CLANG_EXPORTED_TARGETS`
* `clang-19 -print-resource-dir` prints the effective resource dir: 
`/usr/lib/llvm-19/lib/clang/19`
* `CLANG_RESOURCE_DIR` appears to be designed to be a relative path appended to 
`/usr/lib/llvm-19/{bin,lib}`

So I'm thinking even if the `clang-resource-headers` target was available, it 
would not give the path to the resource dir, but rather the path to the subdir 
containing the builtin headers. That might be fine, i.e. I could take that path 
and add it with `-isystem ...`, but if the resource dir is already implicitly 
on the header search path, that seems preferable.


https://github.com/llvm/llvm-project/pull/97197
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] RFC: [cmake] Export CLANG_RESOURCE_DIR in ClangConfig (PR #97197)

[Kim Gräsman via cfe-commits]

kimgr wrote:

> Or making CustomResourceDir = CLANG_RESOURCE_DIR by default instead of "".

Binding that default value in Driver.h would leave it to external projects 
(using Clang) to resolve `CLANG_RESOURCE_DIR`, which is not available outside 
Clang. So it somehow needs to move into the body of `GetResourcesPath`.

Also, looking at the use of `CustomResourceDir`, it looks like it expects path 
relative to `dirname(clang)`, i.e. it would have to be something like ` 
`BinaryPath="/usr/lib/llvm-20/bin/clang"` and 
`{CLANG_RESOURCE_DIR,CustomResourceDir}="../share/my-tool/"`. Overall 
`CLANG_RESOURCE_DIR` and that custom resource dir seems pretty hard to get 
right, I wonder if it's ever used.

It was originally added to the LLVM CMake in 
https://github.com/llvm/llvm-project/commit/ffae4a66fa9de885382c48c725b1039593975986
 and used in the subsequent 
https://github.com/llvm/llvm-project/commit/06067c556a0beb136bf3bb62f478269f67701486.
 I'm curious about these 'strange packaging environments" :)

https://github.com/llvm/llvm-project/pull/97197
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] RFC: [cmake] Export CLANG_RESOURCE_DIR in ClangConfig (PR #97197)

[Kim Gräsman via cfe-commits]

kimgr wrote:

@tstellar @tuliom It seems to me _almost_ all callers pass `CLANG_RESOURCE_DIR` 
for the `CustomResourceDir` optional argument:
https://github.com/search?q=repo%3Allvm%2Fllvm-project%20Driver%3A%3AGetResourcesPath&type=code

the one exception being:
https://github.com/llvm/llvm-project/blob/d72c8b02802c87386f5db3c7de6c79e921618fa3/clang/tools/libclang/CIndexer.cpp#L156

I wonder if that last one is a bug...? If so, it seems it would be better to 
just fold `CLANG_RESOURCE_DIR` into `Driver::GetResourcesPath` and remove the 
optional argument.

https://github.com/llvm/llvm-project/pull/97197
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] RFC: [cmake] Export CLANG_RESOURCE_DIR in ClangConfig (PR #97197)

[Kim Gräsman via cfe-commits]

kimgr wrote:

@etcwilde 
> The path is already available through the exported clang-resource-header 
> imported interface target

I think that one has the full path to the compiler-supplied built-in headers, 
not the resource dir (which is basically its parent). We could do some path 
mangling, but that's what burned us in the first place.


https://github.com/llvm/llvm-project/pull/97197
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] RFC: [cmake] Export CLANG_RESOURCE_DIR in ClangConfig (PR #97197)

[Kim Gräsman via cfe-commits]

kimgr wrote:

> The LLVM_EXTERNAL_PROJECT mode solution using $ won't work 
> so well,
> as it pins the resource directory to the build tree. So it won't resolve 
> correctly after install, I suppose.

Ah, in that mode I guess we could use the default behavior, and not override 
the resource dir.

https://github.com/llvm/llvm-project/pull/97197
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] RFC: [cmake] Export CLANG_RESOURCE_DIR in ClangConfig (PR #97197)

[Kim Gräsman via cfe-commits]

kimgr wrote:

@llvm-beanz Thanks for the pointers!

Here's what I ended up with as a first step:
https://github.com/include-what-you-use/include-what-you-use/commit/b03f60adb2a9fa884d35bea5eb77c9e8a05d80c0

I couldn't get the `get_target_property` spelling you suggested to work (it 
doesn't resolve when building against the Debian packages), so I ended up with 
something else.

The `LLVM_EXTERNAL_PROJECT` mode solution using `$` won't 
work so well, as it pins the resource directory to the build tree. So it won't 
resolve correctly after install, I suppose.

https://github.com/llvm/llvm-project/pull/97197
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] RFC: [cmake] Export CLANG_RESOURCE_DIR in ClangConfig (PR #97197)

[Kim Gräsman via cfe-commits]

kimgr wrote:

@llvm-beanz Thanks! Yes, copying the headers does seem like a hack, in 
retrospect. I think I started down that path because I don't know how packaging 
works -- I had assumed I might have to bundle the headers with IWYU, but I 
guess the right procedure is to build against an installed Clang, wire the 
paths for that installation, and express a dependency in the resulting package?

So is the recommendation to:

* feed path to Clang binary in from CMake via preprocessor symbol or something 
(where do I find that on the CMake side, btw?)
* `resPath = GetResourcesPath(clangBinaryPath)`
* Add a `-resource-dir $resPath` switch as part of driver construction

?

@etcwilde Cool, that might be helpful! I don't need it on the include path, I 
need to somehow forward it into my tool so it will be able to find  
and friends at runtime. Your last example appears to get the path in a form 
where that's possible, thanks!

https://github.com/llvm/llvm-project/pull/97197
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] RFC: [cmake] Export CLANG_RESOURCE_DIR in ClangConfig (PR #97197)

[Kim Gräsman via cfe-commits]

kimgr wrote:

I think at the time we started doing this, Clang's builtin includes lookup 
basically did `$dirname($0)/../lib/llvm-$version/clang/$version/include`. So 
when building `include-what-you-use` in a separate tree, the resulting binary 
didn't have the builtin headers in the right place, and we wouldn't be able to 
run tests.

We support three build modes:

* as part of LLVM+Clang with `LLVM_EXTERNAL_PROJECTS=iwyu` and 
`LLVM_EXTERNAL_IWYU_SOURCE_DIR`
* with `CMAKE_PREFIX_PATH` pointing to a package-installed LLVM+Clang
* with `CMAKE_PREFIX_PATH` pointing to an LLVM+Clang build tree

In the first case, depending on the `Headers` target makes sure all headers are 
copied into the build tree.

For the latter two we synthesize a `Headers` target (because it's not exported) 
based on the Clang resource dir.

https://github.com/llvm/llvm-project/pull/97197
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] RFC: [cmake] Export CLANG_RESOURCE_DIR in ClangConfig (PR #97197)

[Kim Gräsman via cfe-commits]

kimgr wrote:

cc @llvm-beanz -- you seem to have done a fair bit with the LLVM CMake system.

https://github.com/llvm/llvm-project/pull/97197
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] RFC: [cmake] Export CLANG_RESOURCE_DIR in ClangConfig (PR #97197)

[Kim Gräsman via cfe-commits]

https://github.com/kimgr created https://github.com/llvm/llvm-project/pull/97197

I recently opened #95747 to see if it would be advisable to expose 
`CLANG_RESOURCE_DIR` from `ClangConfig.cmake`.

Here's a patch to do the most naive thing I could think of, hopefully enough to 
trigger discussion.

Open questions/concerns:

* `ClangConfig.cmake` now defines `CLANG_RESOURCE_DIR` -- will that interfere 
with the CMake system's `CLANG_RESOURCE_DIR` (which is e.g. baked into config.h)
* The builddir vs. installdir distinction when generate `ClangConfig.cmake` 
isn't 100% clear to me, I guessed a little as to reasonable prefix paths


From 90c3f251342c7452b6d3efc44bb976fc5abc81e4 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Kim=20Gr=C3=A4sman?= 
Date: Sun, 30 Jun 2024 10:45:24 +0200
Subject: [PATCH] RFC: [cmake] Export CLANG_RESOURCE_DIR in ClangConfig

---
 clang/cmake/modules/CMakeLists.txt   | 9 +
 clang/cmake/modules/ClangConfig.cmake.in | 1 +
 2 files changed, 10 insertions(+)

diff --git a/clang/cmake/modules/CMakeLists.txt 
b/clang/cmake/modules/CMakeLists.txt
index d2d68121371bf..4b0a38bd0fc1e 100644
--- a/clang/cmake/modules/CMakeLists.txt
+++ b/clang/cmake/modules/CMakeLists.txt
@@ -2,6 +2,7 @@ include(GNUInstallPackageDir)
 include(ExtendPath)
 include(LLVMDistributionSupport)
 include(FindPrefixFromConfig)
+include(GetClangResourceDir)
 
 # Generate a list of CMake library targets so that other CMake projects can
 # link against them. LLVM calls its version of this file LLVMExports.cmake, but
@@ -29,6 +30,10 @@ set(CLANG_CONFIG_INCLUDE_DIRS
   "${CLANG_SOURCE_DIR}/include"
   "${CLANG_BINARY_DIR}/include"
   )
+
+get_clang_resource_dir(resource_builddir PREFIX ${CLANG_BINARY_DIR})
+set(CLANG_CONFIG_RESOURCE_DIR ${resource_builddir})
+
 configure_file(
   ${CMAKE_CURRENT_SOURCE_DIR}/ClangConfig.cmake.in
   ${clang_cmake_builddir}/ClangConfig.cmake
@@ -60,6 +65,10 @@ extend_path(base_includedir "\${CLANG_INSTALL_PREFIX}" 
"${CMAKE_INSTALL_INCLUDED
 set(CLANG_CONFIG_INCLUDE_DIRS
   "${base_includedir}"
   )
+
+get_clang_resource_dir(resource_installdir PREFIX ${CLANG_INSTALL_PREFIX})
+set(CLANG_CONFIG_RESOURCE_DIR "${resource_installdir}")
+
 configure_file(
   ${CMAKE_CURRENT_SOURCE_DIR}/ClangConfig.cmake.in
   ${CMAKE_CURRENT_BINARY_DIR}/CMakeFiles/ClangConfig.cmake
diff --git a/clang/cmake/modules/ClangConfig.cmake.in 
b/clang/cmake/modules/ClangConfig.cmake.in
index 5f67681649c66..1b97e103c93ca 100644
--- a/clang/cmake/modules/ClangConfig.cmake.in
+++ b/clang/cmake/modules/ClangConfig.cmake.in
@@ -10,6 +10,7 @@ set(CLANG_EXPORTED_TARGETS "@CLANG_EXPORTS@")
 set(CLANG_CMAKE_DIR "@CLANG_CONFIG_CMAKE_DIR@")
 set(CLANG_INCLUDE_DIRS "@CLANG_CONFIG_INCLUDE_DIRS@")
 set(CLANG_LINK_CLANG_DYLIB "@CLANG_LINK_CLANG_DYLIB@")
+set(CLANG_RESOURCE_DIR "@CLANG_CONFIG_RESOURCE_DIR@")
 
 # Provide all our library targets to users.
 @CLANG_CONFIG_INCLUDE_EXPORTS@

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang-tools-extra] [Clang] Unify interface for accessing template arguments as written for class/variable template specializations (PR #81642)

[Kim Gräsman via cfe-commits]

kimgr wrote:

@sdkrystian Thank you! I think there's some more subtleties to it, but I don't 
really have my head around it yet. I'll first try to understand what we're 
trying to achieve and then ping back of I need help with something concrete.

https://github.com/llvm/llvm-project/pull/81642
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang-tools-extra] [Clang] Unify interface for accessing template arguments as written for class/variable template specializations (PR #81642)

[Kim Gräsman via cfe-commits]

kimgr wrote:

@sdkrystian It looks like this PR broke IWYU with respect to this assumption:
> Moreover, we don't ever need the type as written -- in almost all cases, we 
> only want the template arguments (e.g. in tooling use-cases).

As I understand it, IWYU did use the type as written to do resugaring of 
template types, here: 
https://github.com/include-what-you-use/include-what-you-use/blob/master/iwyu_ast_util.cc#L935

Do you have any tips for how to implement that on top of the new model? 
Manually type-switch all TemplateArgument kinds and map to a Type, possibly via 
Expr?

https://github.com/llvm/llvm-project/pull/81642
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Fix the double space and double attribute printing of the final keyword. (PR #88600)

[Kim Gräsman via cfe-commits]

kimgr wrote:

@vgvassilev Thank you!

https://github.com/llvm/llvm-project/pull/88600
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Fix the double space and double attribute printing of the final keyword. (PR #88600)

[Kim Gräsman via cfe-commits]

kimgr wrote:

> I am wondering if that'd be interesting and if so, maybe we can share it 
> between both projects via upstreaming to Clang...

That sounds fantastic, but mostly because I don't have anything to offer, and 
everything to benefit :)

I was just thinking about adding a `.ForwardDeclaration` policy to 
`DeclPrinter` this morning -- the current `PolishForDeclaration` output is _so_ 
close. But it seemed weird to add a mode for which IWYU (a non-LLVM project) 
was the only user.

I suspect your `ForwardDeclarePrinter` is more principled. I guess it might be 
hard to coordinate forward-decl style between users, but ultimately that's just 
printing policy (or chaining libFormat, or something).

Let's maybe continue this discussion somewhere else?

https://github.com/llvm/llvm-project/pull/88600
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Fix the double space and double attribute printing of the final keyword. (PR #88600)

[Kim Gräsman via cfe-commits]

kimgr wrote:

Current PR passes all my tests, both Clang (`ninja ASTTests`), Clangd (`ninja 
ClangdTests`) and IWYU end-to-end tests -- thanks!

https://github.com/llvm/llvm-project/pull/88600
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Fix the double space and double attribute printing of the final keyword. (PR #88600)

[Kim Gräsman via cfe-commits]

@@ -252,16 +252,19 @@ static DeclPrinter::AttrPosAsWritten 
getPosAsWritten(const Attr *A,
   return DeclPrinter::AttrPosAsWritten::Right;
 }
 
-void DeclPrinter::prettyPrintAttributes(const Decl *D,
+// returns true if an attribute was printed.
+bool DeclPrinter::prettyPrintAttributes(const Decl *D,
 AttrPosAsWritten Pos /*=Default*/) {
-  if (Policy.PolishForDeclaration)
-return;
+  bool hasPrinted = false;
 
   if (D->hasAttrs()) {
 const AttrVec &Attrs = D->getAttrs();
 for (auto *A : Attrs) {
   if (A->isInherited() || A->isImplicit())
 continue;
+  // Don't strip out the keyword attributes, they aren't regular 
attributes.
+  if (Policy.PolishForDeclaration && !A->isKeywordAttribute())

kimgr wrote:

Nice, I was about to suggest something like this, but didn't know 
`isKeywordAttribute` existed.

https://github.com/llvm/llvm-project/pull/88600
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Fix the double space and double attribute printing of the final keyword. (PR #88600)

[Kim Gräsman via cfe-commits]

kimgr wrote:

> If the intent is to produce a forward declaration the final keyword cannot be 
> attached to a forward declaration. So I am not sure what's the "right" fix 
> here...

I don't believe that's the intent of `DeclPrinter` or `PolishForDeclaration` --

* Clangd uses `PolishForDeclaration` to print an informational "hover text" for 
the declaration (and I guess that's why they want to include `final` -- it's 
something that's good for an interactive user to know about the decl)
* IWYU uses `PolishForDeclaration` to get a valid _declaration_, and then does 
[some very hacky heuristic 
post-processing](https://github.com/include-what-you-use/include-what-you-use/blob/125341c412ceee9233ece8973848b49e770a9b82/iwyu_output.cc#L469)
 to turn it into a forward-decl.

Sorry for stirring confusion into this, my primary focus is IWYU, but I 
remember that the original double-final came from a change intended for Clangd.

Hopefully this helps clarify.

https://github.com/llvm/llvm-project/pull/88600
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Fix the double space and double attribute printing of the final keyword. (PR #88600)

[Kim Gräsman via cfe-commits]

kimgr wrote:

Thank you.

I believe this should cover both cases, added some attempt at rationale in 
comments:
```diff
diff --git a/clang/unittests/AST/DeclPrinterTest.cpp 
b/clang/unittests/AST/DeclPrinterTest.cpp
index f2b027a25621..8691a6c38f16 100644
--- a/clang/unittests/AST/DeclPrinterTest.cpp
+++ b/clang/unittests/AST/DeclPrinterTest.cpp
@@ -86,16 +86,13 @@ PrintedDeclCXX98Matches(StringRef Code, const 
DeclarationMatcher &NodeMatch,
 ExpectedPrinted, "input.cc");
 }
 
-::testing::AssertionResult PrintedDeclCXX11Matches(
-  StringRef Code,
-  const DeclarationMatcher &NodeMatch,
-  StringRef ExpectedPrinted) {
+::testing::AssertionResult
+PrintedDeclCXX11Matches(StringRef Code, const DeclarationMatcher &NodeMatch,
+StringRef ExpectedPrinted,
+PrintingPolicyAdjuster PolicyModifier = nullptr) {
   std::vector Args(1, "-std=c++11");
-  return PrintedDeclMatches(Code,
-Args,
-NodeMatch,
-ExpectedPrinted,
-"input.cc");
+  return PrintedDeclMatches(Code, Args, NodeMatch, ExpectedPrinted, "input.cc",
+PolicyModifier);
 }
 
 ::testing::AssertionResult PrintedDeclCXX11nonMSCMatches(
@@ -1556,3 +1553,25 @@ TEST(DeclPrinter, VarDeclWithInitializer) {
   PrintedDeclCXX17Matches("void foo() {int arr[42]; for(int a : arr);}",
   namedDecl(hasName("a")).bind("id"), "int a"));
 }
+
+TEST(DeclPrinter, TestTemplateFinal) {
+  // By default we should print 'final' keyword whether class is implicitly or
+  // explicitly marked final.
+  ASSERT_TRUE(PrintedDeclCXX11Matches(
+  "template\n"
+  "class FinalTemplate final {};",
+  classTemplateDecl(hasName("FinalTemplate")).bind("id"),
+  "template  class FinalTemplate final {}"));
+}
+
+TEST(DeclPrinter, TestTemplateFinalWithPolishForDecl) {
+  // clangd relies on the 'final' keyword being printed when
+  // PolishForDeclaration is enabled, so make sure it is even if implicit attrs
+  // are disabled.
+  ASSERT_TRUE(PrintedDeclCXX11Matches(
+  "template\n"
+  "class FinalTemplate final {};",
+  classTemplateDecl(hasName("FinalTemplate")).bind("id"),
+  "template  class FinalTemplate final {}",
+  [](PrintingPolicy &Policy) { Policy.PolishForDeclaration = true; }));
+}
```

https://github.com/llvm/llvm-project/pull/88600
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Fix the double space and double attribute printing of the final keyword. (PR #88600)

[Kim Gräsman via cfe-commits]

kimgr wrote:

> With .PolishForDeclaration=true, there are NO final specifiers (which is what 
> we want to produce forward decls in IWYU)

This is actually a regression in this PR, and it breaks the clangd test added 
here: 
https://github.com/llvm/llvm-project/commit/9f57b65a272817752aa00e2fb94154e6eed1d0ec
 (the patch that originally led to double `final`s).


https://github.com/llvm/llvm-project/pull/88600
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Fix the double space and double attribute printing of the final keyword. (PR #88600)

[Kim Gräsman via cfe-commits]

kimgr wrote:

Thanks!

Could you add this DeclPrinterTest unittest for regression?
```
TEST(DeclPrinter, TestTemplateFinal) {
  ASSERT_TRUE(PrintedDeclCXX11Matches(
"template"
"class FinalTemplate final {};",
classTemplateDecl(hasName("FinalTemplate")).bind("id"),
"template  class FinalTemplate final {}"));
}
```
This is my expectation for correct output, but the current branch seems to 
disagree -- now there's two spaces between `final` and `{}`. Not sure if that's 
a problem. IWYU has enough post-processing that it doesn't break us either way.

https://github.com/llvm/llvm-project/pull/88600
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Rework the printing of attributes (PR #87281)

[Kim Gräsman via cfe-commits]

kimgr wrote:

Ah, that's the expected output -- I can't do anything about that :). See 
https://github.com/llvm/llvm-project/issues/56517.

https://github.com/llvm/llvm-project/pull/87281
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Rework the printing of attributes (PR #87281)

[Kim Gräsman via cfe-commits]

kimgr wrote:

@vgvassilev I did expect the input to be valid, yes:
```
template
class FinalTemplate final {};
```
Is it not?

https://github.com/llvm/llvm-project/pull/87281
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Rework the printing of attributes (PR #87281)

[Kim Gräsman via cfe-commits]

kimgr wrote:

(obtw, the double `final` is unrelated, it's tracked here: 
https://github.com/llvm/llvm-project/issues/56517)

https://github.com/llvm/llvm-project/pull/87281
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Rework the printing of attributes (PR #87281)

[Kim Gräsman via cfe-commits]

kimgr wrote:

I can confirm that the double space comes from this PR;
```diff
diff --git a/clang/unittests/AST/DeclPrinterTest.cpp 
b/clang/unittests/AST/DeclPrinterTest.cpp
index c24e442621c9..c2d02e74a62c 100644
--- a/clang/unittests/AST/DeclPrinterTest.cpp
+++ b/clang/unittests/AST/DeclPrinterTest.cpp
@@ -1555,3 +1555,11 @@ TEST(DeclPrinter, VarDeclWithInitializer) {
   PrintedDeclCXX17Matches("void foo() {int arr[42]; for(int a : arr);}",
   namedDecl(hasName("a")).bind("id"), "int a"));
 }
+
+TEST(DeclPrinter, TestTemplateFinal) {
+  ASSERT_TRUE(PrintedDeclCXX11Matches(
+  "template\n"
+  "class FinalTemplate final {};",
+  classTemplateDecl(hasName("FinalTemplate")).bind("id"),
+  "template  class final FinalTemplate final {}"));
+}
```
fails with:
```
.../llvm-project/clang/unittests/AST/DeclPrinterTest.cpp:1560: [...]
  Expected "template  class final FinalTemplate final {}"
  got   "template  class  final FinalTemplate final {}")
```
(edited so it's easier to see the diff).

It passes after I revert the `Rework attributes` commits:

* 9391ff8c86007562d40c240ea082b7c0cbf35947
* 62e92573d28d62ab7e6438ac34d513b07c51ce09
* a30662fc2acdd73ca1a9217716299a4676999fb4

Again, from my point of view, this "bug" is fine on mainline, we can work 
around it. But it would be nice if this patch was not backported to 18 as it 
breaks our corresponding release.

https://github.com/llvm/llvm-project/pull/87281
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Rework the printing of attributes (PR #87281)

[Kim Gräsman via cfe-commits]

kimgr wrote:

@erichkeane Thanks! Let me just see if I can bisect it to this commit; I don't 
have any evidence yet, this PR was just the only significant change to 
`DeclPrinter.cpp` in the past few days. I need a little while to rebuild my 
local Clang tree with and without the patch.

https://github.com/llvm/llvm-project/pull/87281
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Rework the printing of attributes (PR #87281)

[Kim Gräsman via cfe-commits]

kimgr wrote:

A note from left field: I think this PR broke the IWYU test suite. We use 
`TemplateDecl::print` + some postprocessing to generate template 
forward-declarations. We're seeing this diff now:
```diff
-template  class FinalTemplate;
+template  class  FinalTemplate;
```
So a spurious extra space.

We would be grateful if this _didn't_ make the 18.x branches, because we've 
already released an 18.x-compatible version of IWYU, and it would be a shame if 
that release had a failing test suite when built against later 18.x Clang.

I'll work around this on our latest mainline.

https://github.com/llvm/llvm-project/pull/87281
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [clang] 3ced239 - Refactor CompareReferenceRelationship and its callers in preparation for

[Kim Gräsman via cfe-commits]

Yes, it is this change that broke us.

I still don't fully understand what the change was, but it appears to
mess things up for IWYU for operator== with templates and const,
somehow.

Could you expand on what changed here and how the AST might be affected?

Thanks,
- Kim

On Sat, Dec 28, 2019 at 4:35 PM Kim Gräsman  wrote:
>
> Hi Richard,
>
> I see the commit message mentions type sugar here; does this change
> affect the AST at all?
>
> We're seeing test failures in IWYU based on recent Clang, and I'm
> suspecting this commit (it takes me a while to bisect because of Clang
> build times on my laptop).
>
> Thanks,
> - Kim
>
> On Wed, Dec 18, 2019 at 11:06 PM Richard Smith via cfe-commits
>  wrote:
> >
> >
> > Author: Richard Smith
> > Date: 2019-12-18T14:05:57-08:00
> > New Revision: 3ced23976aa8a86a17017c87821c873b4ca80bc2
> >
> > URL: 
> > https://github.com/llvm/llvm-project/commit/3ced23976aa8a86a17017c87821c873b4ca80bc2
> > DIFF: 
> > https://github.com/llvm/llvm-project/commit/3ced23976aa8a86a17017c87821c873b4ca80bc2.diff
> >
> > LOG: Refactor CompareReferenceRelationship and its callers in preparation 
> > for
> > implementing the resolution of CWG2352.
> >
> > No functionality change, except that we now convert the referent of a
> > reference binding to the underlying type of the reference in more cases;
> > we used to happen to preserve the type sugar from the referent if the
> > only type change was in the cv-qualifiers.
> >
> > This exposed a bug in how we generate code for trivial assignment
> > operators: if the type sugar (particularly the may_alias attribute)
> > got lost during reference binding, we'd use the "wrong" TBAA information
> > for the load during the assignment.
> >
> > Added:
> >
> >
> > Modified:
> > clang/include/clang/Sema/Sema.h
> > clang/lib/CodeGen/CGExprCXX.cpp
> > clang/lib/Sema/SemaCast.cpp
> > clang/lib/Sema/SemaExprCXX.cpp
> > clang/lib/Sema/SemaInit.cpp
> > clang/lib/Sema/SemaOverload.cpp
> > clang/test/AST/ast-dump-expr-json.cpp
> > clang/test/Index/print-type.cpp
> >
> > Removed:
> >
> >
> >
> > 
> > diff  --git a/clang/include/clang/Sema/Sema.h 
> > b/clang/include/clang/Sema/Sema.h
> > index 2730eef0bdd8..07eba0306c98 100755
> > --- a/clang/include/clang/Sema/Sema.h
> > +++ b/clang/include/clang/Sema/Sema.h
> > @@ -31,6 +31,7 @@
> >  #include "clang/AST/StmtCXX.h"
> >  #include "clang/AST/TypeLoc.h"
> >  #include "clang/AST/TypeOrdering.h"
> > +#include "clang/Basic/BitmaskEnum.h"
> >  #include "clang/Basic/ExpressionTraits.h"
> >  #include "clang/Basic/Module.h"
> >  #include "clang/Basic/OpenMPKinds.h"
> > @@ -10703,11 +10704,26 @@ class Sema final {
> >  Ref_Compatible
> >};
> >
> > +  // Fake up a scoped enumeration that still contextually converts to bool.
> > +  struct ReferenceConversionsScope {
> > +/// The conversions that would be performed on an lvalue of type T2 
> > when
> > +/// binding a reference of type T1 to it, as determined when evaluating
> > +/// whether T1 is reference-compatible with T2.
> > +enum ReferenceConversions {
> > +  Qualification = 0x1,
> > +  Function = 0x2,
> > +  DerivedToBase = 0x4,
> > +  ObjC = 0x8,
> > +  ObjCLifetime = 0x10,
> > +
> > +  LLVM_MARK_AS_BITMASK_ENUM(/*LargestValue=*/ObjCLifetime)
> > +};
> > +  };
> > +  using ReferenceConversions = 
> > ReferenceConversionsScope::ReferenceConversions;
> > +
> >ReferenceCompareResult
> >CompareReferenceRelationship(SourceLocation Loc, QualType T1, QualType 
> > T2,
> > -   bool &DerivedToBase, bool &ObjCConversion,
> > -   bool &ObjCLifetimeConversion,
> > -   bool &FunctionConversion);
> > +   ReferenceConversions *Conv = nullptr);
> >
> >ExprResult checkUnknownAnyCast(SourceRange TypeRange, QualType CastType,
> >   Expr *CastExpr, CastKind &CastKind,
> >
> > diff  --git a/clang/lib/CodeGen/CGExprCXX.cpp 
> > b/clang/lib/CodeGen/CGExprCXX.cpp
> > index 269b80b43403..3fc86136c529 100644
> > --- a/clang/lib/CodeGen/CGExprCXX.cpp
> > +++ b/clang/lib/CodeGen/CGExprCXX.cpp
> > @@ -241,16 +241,28 @@ RValue 
> > CodeGenFunction::EmitCXXMemberOrOperatorMemberCallExpr(
> >  }
> >}
> >
> > +  bool TrivialForCodegen =
> > +  MD->isTrivial() || (MD->isDefaulted() && MD->getParent()->isUnion());
> > +  bool TrivialAssignment =
> > +  TrivialForCodegen &&
> > +  (MD->isCopyAssignmentOperator() || MD->isMoveAssignmentOperator()) &&
> > +  !MD->getParent()->mayInsertExtraPadding();
> > +
> >// C++17 demands that we evaluate the RHS of a (possibly-compound) 
> > assignment
> >// operator before the LHS.
> >CallArgList RtlArgStorage;
> >CallArgList *RtlArgs = nullptr;
> > +  LValue TrivialAssignmentRHS;
> >if (auto *OCE = dyn_cast(CE

Re: [clang] 3ced239 - Refactor CompareReferenceRelationship and its callers in preparation for

[Kim Gräsman via cfe-commits]

Hi Richard,

I see the commit message mentions type sugar here; does this change
affect the AST at all?

We're seeing test failures in IWYU based on recent Clang, and I'm
suspecting this commit (it takes me a while to bisect because of Clang
build times on my laptop).

Thanks,
- Kim

On Wed, Dec 18, 2019 at 11:06 PM Richard Smith via cfe-commits
 wrote:
>
>
> Author: Richard Smith
> Date: 2019-12-18T14:05:57-08:00
> New Revision: 3ced23976aa8a86a17017c87821c873b4ca80bc2
>
> URL: 
> https://github.com/llvm/llvm-project/commit/3ced23976aa8a86a17017c87821c873b4ca80bc2
> DIFF: 
> https://github.com/llvm/llvm-project/commit/3ced23976aa8a86a17017c87821c873b4ca80bc2.diff
>
> LOG: Refactor CompareReferenceRelationship and its callers in preparation for
> implementing the resolution of CWG2352.
>
> No functionality change, except that we now convert the referent of a
> reference binding to the underlying type of the reference in more cases;
> we used to happen to preserve the type sugar from the referent if the
> only type change was in the cv-qualifiers.
>
> This exposed a bug in how we generate code for trivial assignment
> operators: if the type sugar (particularly the may_alias attribute)
> got lost during reference binding, we'd use the "wrong" TBAA information
> for the load during the assignment.
>
> Added:
>
>
> Modified:
> clang/include/clang/Sema/Sema.h
> clang/lib/CodeGen/CGExprCXX.cpp
> clang/lib/Sema/SemaCast.cpp
> clang/lib/Sema/SemaExprCXX.cpp
> clang/lib/Sema/SemaInit.cpp
> clang/lib/Sema/SemaOverload.cpp
> clang/test/AST/ast-dump-expr-json.cpp
> clang/test/Index/print-type.cpp
>
> Removed:
>
>
>
> 
> diff  --git a/clang/include/clang/Sema/Sema.h 
> b/clang/include/clang/Sema/Sema.h
> index 2730eef0bdd8..07eba0306c98 100755
> --- a/clang/include/clang/Sema/Sema.h
> +++ b/clang/include/clang/Sema/Sema.h
> @@ -31,6 +31,7 @@
>  #include "clang/AST/StmtCXX.h"
>  #include "clang/AST/TypeLoc.h"
>  #include "clang/AST/TypeOrdering.h"
> +#include "clang/Basic/BitmaskEnum.h"
>  #include "clang/Basic/ExpressionTraits.h"
>  #include "clang/Basic/Module.h"
>  #include "clang/Basic/OpenMPKinds.h"
> @@ -10703,11 +10704,26 @@ class Sema final {
>  Ref_Compatible
>};
>
> +  // Fake up a scoped enumeration that still contextually converts to bool.
> +  struct ReferenceConversionsScope {
> +/// The conversions that would be performed on an lvalue of type T2 when
> +/// binding a reference of type T1 to it, as determined when evaluating
> +/// whether T1 is reference-compatible with T2.
> +enum ReferenceConversions {
> +  Qualification = 0x1,
> +  Function = 0x2,
> +  DerivedToBase = 0x4,
> +  ObjC = 0x8,
> +  ObjCLifetime = 0x10,
> +
> +  LLVM_MARK_AS_BITMASK_ENUM(/*LargestValue=*/ObjCLifetime)
> +};
> +  };
> +  using ReferenceConversions = 
> ReferenceConversionsScope::ReferenceConversions;
> +
>ReferenceCompareResult
>CompareReferenceRelationship(SourceLocation Loc, QualType T1, QualType T2,
> -   bool &DerivedToBase, bool &ObjCConversion,
> -   bool &ObjCLifetimeConversion,
> -   bool &FunctionConversion);
> +   ReferenceConversions *Conv = nullptr);
>
>ExprResult checkUnknownAnyCast(SourceRange TypeRange, QualType CastType,
>   Expr *CastExpr, CastKind &CastKind,
>
> diff  --git a/clang/lib/CodeGen/CGExprCXX.cpp 
> b/clang/lib/CodeGen/CGExprCXX.cpp
> index 269b80b43403..3fc86136c529 100644
> --- a/clang/lib/CodeGen/CGExprCXX.cpp
> +++ b/clang/lib/CodeGen/CGExprCXX.cpp
> @@ -241,16 +241,28 @@ RValue 
> CodeGenFunction::EmitCXXMemberOrOperatorMemberCallExpr(
>  }
>}
>
> +  bool TrivialForCodegen =
> +  MD->isTrivial() || (MD->isDefaulted() && MD->getParent()->isUnion());
> +  bool TrivialAssignment =
> +  TrivialForCodegen &&
> +  (MD->isCopyAssignmentOperator() || MD->isMoveAssignmentOperator()) &&
> +  !MD->getParent()->mayInsertExtraPadding();
> +
>// C++17 demands that we evaluate the RHS of a (possibly-compound) 
> assignment
>// operator before the LHS.
>CallArgList RtlArgStorage;
>CallArgList *RtlArgs = nullptr;
> +  LValue TrivialAssignmentRHS;
>if (auto *OCE = dyn_cast(CE)) {
>  if (OCE->isAssignmentOp()) {
> -  RtlArgs = &RtlArgStorage;
> -  EmitCallArgs(*RtlArgs, MD->getType()->castAs(),
> -   drop_begin(CE->arguments(), 1), CE->getDirectCallee(),
> -   /*ParamsToSkip*/0, EvaluationOrder::ForceRightToLeft);
> +  if (TrivialAssignment) {
> +TrivialAssignmentRHS = EmitLValue(CE->getArg(1));
> +  } else {
> +RtlArgs = &RtlArgStorage;
> +EmitCallArgs(*RtlArgs, MD->getType()->castAs(),
> + drop_begin(CE->arguments(), 1), CE->getDirectCallee(),
> +  

Re: r368874 - Document clang-cpp in the release notes for clang

[Kim Gräsman via cfe-commits]

On Wed, Aug 14, 2019 at 6:48 PM Chris Bieneman via cfe-commits
 wrote:
>
> Author: cbieneman
> Date: Wed Aug 14 09:49:52 2019
> New Revision: 368874
> --  ...
> +- In 9.0.0 and later Clang added a new target, clang-cpp, which generates a
> +  shared library comprised of all the clang component libraries and exporting
> +  the clang C++ APIs. Additionally the build system gained the new
> +  "CLANG_LINK_CLANG_DYLIB" option, which defaults Off, and when set to On, 
> will
> +  force clang (and clang-based tools) to link the clang-cpp library instead 
> of
> +  statically linking clang's components. This option will reduce the size of
> +  binary distributions at the expense of compiler performance.

Does this also work for Windows/MSVC builds?

Thanks,
- Kim
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D61909: Add Clang shared library with C++ exports

[Kim Gräsman via cfe-commits]

The Clang module libraries are all called libClang[A-Z][a-zA-Z]+.{a,so}, so
libclangcpp doesn't conflict with that, but I wonder if a dash would set it
apart even more clearly: libclang-cpp.

Or something like clang-all to show that it houses all clang modules?

Bikesheds are the best sheds.

- Kim

On Tue, Jul 2, 2019, 10:44 Sylvestre Ledru  wrote:

> libclangcpp ?
>
> I think it is a pretty common pattern.
>
> On debian, apt-cache search 'lib.*cpp'  returns a bunch of libraries
> (libhdf5-cpp, libroscpp2d, libjsonrpccpp-dev, libmysqlcppconn7v5,
> libsvncpp3, libtercpp0v5, libyaml-cpp-dev, etc)
>
> S
>
>
> Le 02/07/2019 à 01:22, Chris Bieneman a écrit :
>
> The question is, what *should* it be called.
>
> While yes, the 's' in 'so' is shared, the "dylib" and "dll" extensions on
> Darwin and Windows have the same meaning too. The problem is libclang.so is
> already taken.
>
> I'm not attached to the name in any way, so I'm open to suggestions.
>
> We do have documentation on best practices for how to build distributions,
> which includes explanations of how to pick and choose what you want to
> install (http://llvm.org/docs/BuildingADistribution.html), so you
> shouldn't need an option to disable it.
>
> -Chris
>
> On Jul 1, 2019, at 6:13 PM, Tom Stellard via Phabricator <
> revi...@reviews.llvm.org> wrote:
>
> tstellar added a comment.
>
> In D61909#1563678 ,
> @sylvestre.ledru wrote:
>
> For now, it isn't part of the debian packaging.
>
> https://salsa.debian.org/pkg-llvm-team/llvm-toolchain/blob/snapshot/debian/rules#L563
> it is removed as packaging phase as I have been told it isn't ready.
>
> Anyway, the lib should not keep this name.
> By definition, on linux, .so means shared. It should have a more explicit
> name.
>
> @beanz Can you please rename it? thanks
>
>
>
> I've filed a bug for this and marked it as a blocker for 9.0.0, because
> once we ship a release with this name, it will be harder to change:
> https://bugs.llvm.org/show_bug.cgi?id=42475
>
>
> Repository:
>  rC Clang
>
> CHANGES SINCE LAST ACTION
>  https://reviews.llvm.org/D61909/new/
>
> https://reviews.llvm.org/D61909
>
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D53354: [WIP][NOT FOR COMMIT][PROTOTYPE] clang-scan-deps: dependency scanning tool rough prototype

[Kim Gräsman via cfe-commits]

Hi Alex,

On Fri, Oct 19, 2018 at 1:11 AM Alex Lorenz via Phabricator via
cfe-commits  wrote:
>
> > Have you checked the tool //Include What You Use//? I'm curious in what way 
> > the mishappenings of that tool present themselves in yours. There were some 
> > challenges not overcome in a good way in IWYU, their readme goes into a bit 
> > more detail on this.
>
> I haven't looked into IWYU that much. Could you please elaborate on which 
> mishappenings you think might present themselves here?

IWYU maintainer here.

I'm not sure exactly what your tool wants to do, or what Whisperity
meant, but we have a couple of doc pages on why IWYU is tricky:
https://github.com/include-what-you-use/include-what-you-use/blob/master/docs/WhatIsAUse.md
https://github.com/include-what-you-use/include-what-you-use/blob/master/docs/WhyIWYUIsDifficult.md

FWIW,
- Kim
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r337381 - Mention clang-cl improvements from r335466 and r336379 in ReleaseNotes.rst

[Kim Gräsman via cfe-commits]

This is lovely! Found a bit inline...

On Wed, Jul 18, 2018, 14:00 Nico Weber via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> Author: nico
> Date: Wed Jul 18 04:55:03 2018
> New Revision: 337381
>
> URL: http://llvm.org/viewvc/llvm-project?rev=337381&view=rev
> Log:
> Mention clang-cl improvements from r335466 and r336379 in ReleaseNotes.rst
>
> Modified:
> cfe/trunk/docs/ReleaseNotes.rst
>
> Modified: cfe/trunk/docs/ReleaseNotes.rst
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/ReleaseNotes.rst?rev=337381&r1=337380&r2=337381&view=diff
>
> ==
> --- cfe/trunk/docs/ReleaseNotes.rst (original)
> +++ cfe/trunk/docs/ReleaseNotes.rst Wed Jul 18 04:55:03 2018
> @@ -166,7 +166,19 @@ Attribute Changes in Clang
>  Windows Support
>  ---
>
> -Clang's support for building native Windows programs ...
> +- clang-cl's support for precompiled headers has been much improved:
> +
> +   - When using a pch file, clang-cl now no longer redundantly emits
> inline
> + methods that are already stored in the obj that was built together
> with
> + the pch file (matching cl.exe).  This speeds up builds using pch
> files
> + by around 30%.
> +
> +   - The /Ycfoo.h and /Yufoo.h flags an now be used without /FIfoo.h when
>
> typo: an - > can
>
> - Kim
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r330068 - [Serialization] Fix some Clang-tidy modernize and Include What You Use warnings; other minor fixes (NFC).

[Kim Gräsman via cfe-commits]

That would be a nice outcome of all the "run-tools-on-llvm" changes if any
problems were filed as bugs on the tools. We have a number of them filed on
iwyu, and they make for nice, concrete bugs to troubleshoot even if we
don't always know how to fix them.

For this specific clang-tidy issue, do you have any ideas for how to tell
this loop apart from any other? I'm guessing the container is modified
while iterating... Or do you mean skip all non-iterator loops?

- Kim

Den lör 14 apr. 2018 12:20Malcolm Parsons via cfe-commits <
cfe-commits@lists.llvm.org> skrev:

> On Sat, 14 Apr 2018, 04:22 Richard Trieu via cfe-commits, <
> cfe-commits@lists.llvm.org> wrote:
>
>> I was tracking down a similar issue to the lldb issue before noticing the
>> change was reverted.  The bad change that lead to it is:
>>
>>  // Load pending declaration chains.
>> -for (unsigned I = 0; I != PendingDeclChains.size(); ++I)
>> -  loadPendingDeclChain(PendingDeclChains[I].first,
>> PendingDeclChains[I].second);
>> +for (const auto &I : PendingDeclChains)
>> +  loadPendingDeclChain(I.first, I.second);
>>  PendingDeclChains.clear();
>>
>> Although the two looks like similar, the vector PendingDeclChains is a
>> class member and gets new elements during loop runs.  Once enough elements
>> are added to the vector, it get reallocated to a larger memory, but the
>> loop is still trying to process the old, now freed, memory.  Using an index
>> and checking the size every loop is the right way to process this vector.
>>
>
> Should clang-tidy handle this type of loop differently?
>
> --
> Malcolm Parsons
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D43578: -ftime-report switch support in Clang

[Kim Gräsman via cfe-commits]

Hi Richard,

On Sat, Apr 14, 2018 at 1:53 AM, Richard Smith - zygoloid via
Phabricator via cfe-commits  wrote:
> rsmith added a comment.
>
> So I don't think this patch is reasonable for that reason. I'm also not sure 
> whether this, as is, is addressing a pressing use case -- for Clang 
> developers, existing non-invasive profiling tools (such as linux-perftools) 
> are likely to work a lot better for identifying where in the Clang source 
> code we're spending time. And Clang users typically don't care which function 
> we're in (unless they're filing a bug, where again a profiler is probably a 
> better tool).
>
> However, I do think we could make `-ftime-report` vastly more useful to Clang 
> users. Specifically, I think the useful, actionable feedback we can give to 
> users would be to tell them which parts of //their source code// are taking a 
> long time to compile. Profiling information that can describe -- for instance 
> -- the time spent instantiating the N slowest templates, or handling the N 
> slowest functions, or evaluating the N slowest constant expressions, or 
> parsing the N slowest `#include`d files, seems like it would be incredibly 
> valuable. To make that work, I think we'll need to teach LLVM's timer 
> infrastructure to properly separate out "self" time from "children" time for 
> timers in the same group, and may need other infrastructure improvements.

I disagreed up until the last paragraph :)

That's exactly the crux of what most users need to know -- which parts
of my source code are causing the biggest build slow-down? The summary
information from -ftime-report can give a hint, but a detailed
breakdown would of course be great!

- Kim
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D44602: [clang-tidy] readability-function-size: add VariableThreshold param.

[Kim Gräsman via cfe-commits]

I have to say I disagree that either the nested struct/function or macros
(in any form) should count toward a function's total variable count.

Both are valid forms of abstraction, and they both remove complexity from
the containing function since they factor details *out of the function's
immediate lexical contents* (I avoid 'scope' as macros do pollute the
scope) in a way that improves readability.

There are cases where macros can make things more complex but it seems
unfair to flag variables declared by macros as making expanding functions
more complex.

In short, I think I agree with Aaron's last example classifications.

- Kim


Den tors 22 mars 2018 14:56Eugene Zelenko via Phabricator via cfe-commits <
cfe-commits@lists.llvm.org> skrev:

> Eugene.Zelenko added inline comments.
>
>
> 
> Comment at: docs/ReleaseNotes.rst:127
>
> +- Added `VariableThreshold` option to `readability-function-size
> +  <
> http://clang.llvm.org/extra/clang-tidy/checks/readability-function-size.html>`_
> check
> 
> Please rebase from trunk and use //:doc:// for link.
>
>
> Repository:
>   rCTE Clang Tools Extra
>
> https://reviews.llvm.org/D44602
>
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D30170: Function definition may have uninstantiated body

[Kim Gräsman via cfe-commits]

On Wed, Feb 28, 2018 at 8:21 PM, Richard Smith - zygoloid via
Phabricator via cfe-commits  wrote:
> 
> Comment at: lib/Sema/SemaDecl.cpp:11986
> +  !FD->isDefined(Definition)
> +  && FD->getDeclContext()->isFileContext()) {
> +// If this is a friend function defined in a class template, it does not
> 
> `&&` on the end of the previous line, please.
>
> If the intent here is to detect non-member functions, using 
> `!FD->isCXXClassMember()` or `!isa(FD)` would be clearer.

I just recently did something like this and finally settled on
`FD->getKind() == Decl::Function`. `FunctionDecl` now has two derived
types, `CXXMethodDecl` and `CXXDeductionGuideDecl`, so the negation on
`CXXMethodDecl` is no longer strictly enough.

I think it would be nice if `FunctionDecl` had an
`isNonMemberFunction()` (or whatever the formal term is) that did the
above-mentioned check for kind equality.

For what it's worth,
- Kim
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r321312 - [AST] Incorrectly qualified unscoped enumeration as template actual parameter.

[Kim Gräsman via cfe-commits]

This broke a test case in IWYU, so I read the diff a few times more
than usual...

On Thu, Dec 21, 2017 at 10:47 PM, Paul Robinson via cfe-commits
 wrote:
>
>// scope of the enumeration.
> -  if (ED->isScoped() || ED->getIdentifier())
> +  // For the case of unscoped enumerator, do not include in the qualified
> +  // name any information about its enum enclosing scope, as is 
> visibility
> +  // is global.


Typo: as is -> as its

Happy holidays,
- Kim
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [clang-tools-extra] r318809 - Silence some MSVC warnings about not all control paths returning a value; NFC.

[Kim Gräsman via cfe-commits]

Den 21 nov. 2017 11:24 em skrev "Aaron Ballman via cfe-commits" <
cfe-commits@lists.llvm.org>:

Author: aaronballman
Date: Tue Nov 21 14:24:13 2017
New Revision: 318809

URL: http://llvm.org/viewvc/llvm-project?rev=318809&view=rev
Log:
Silence some MSVC warnings about not all control paths returning a value;
NFC.

Modified:
clang-tools-extra/trunk/clangd/JSONExpr.cpp
clang-tools-extra/trunk/clangd/JSONExpr.h

Modified: clang-tools-extra/trunk/clangd/JSONExpr.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/
trunk/clangd/JSONExpr.cpp?rev=318809&r1=318808&r2=318809&view=diff

==
--- clang-tools-extra/trunk/clangd/JSONExpr.cpp (original)
+++ clang-tools-extra/trunk/clangd/JSONExpr.cpp Tue Nov 21 14:24:13 2017
@@ -519,6 +519,7 @@ bool operator==(const Expr &L, const Exp
   case Expr::Object:
 return *L.object() == *R.object();
   }
+  llvm_unreachable("Unknown expressiopn kind");


Typo: expressiopn. Though it *is* unreachable :)

- Kim
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D39360: [C++11] Don't put empty quotes in static_assert diagnostic.

[Kim Gräsman via cfe-commits]

A clang-tidy check to remove empty messages from source would be nice,
though...

- Kim

Den 27 okt. 2017 10:24 fm skrev "Nicolas Lesser via Phabricator" <
revi...@reviews.llvm.org>:

> Rakete abandoned this revision.
> Rakete added a comment.
>
> @kimgr Well, mostly because they bother me a bit, don't know what others
> think though. I just thought it would be nice if they didn't appear, mainly
> because there is no need to show empty quotes in the error message. Hmm,
> you have a point though... Didn't think of that. Thanks +1
>
>
> https://reviews.llvm.org/D39360
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [clang-tools-extra] r304583 - [clang-tidy] Add `const` to operator() to fix a warning.

[Kim Gräsman via cfe-commits]

This is even an error in VS2017, I've just fixed a number of instances of
this in an internal codebase.

- Kim

Den 6 juni 2017 4:32 em skrev "Alexander Kornienko via cfe-commits" <
cfe-commits@lists.llvm.org>:

> On Mon, Jun 5, 2017 at 7:11 PM, David Blaikie  wrote:
>
>> what was the warning?
>>
>
> I don't remember the exact warning text, but the idea was that a non-const
> operator() could not be called. The change is reasonable in any case: the
> operator() here has no reason to be non-const.
>
>
>>
>> On Fri, Jun 2, 2017 at 11:48 AM Alexander Kornienko via cfe-commits <
>> cfe-commits@lists.llvm.org> wrote:
>>
>>> Author: alexfh
>>> Date: Fri Jun  2 13:47:50 2017
>>> New Revision: 304583
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=304583&view=rev
>>> Log:
>>> [clang-tidy] Add `const` to operator() to fix a warning.
>>>
>>> Modified:
>>> clang-tools-extra/trunk/clang-tidy/misc/LambdaFunctionNameCheck.h
>>>
>>> Modified: clang-tools-extra/trunk/clang-tidy/misc/LambdaFunctionNameCh
>>> eck.h
>>> URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/
>>> clang-tidy/misc/LambdaFunctionNameCheck.h?rev=304583&r1=
>>> 304582&r2=304583&view=diff
>>> 
>>> ==
>>> --- clang-tools-extra/trunk/clang-tidy/misc/LambdaFunctionNameCheck.h
>>> (original)
>>> +++ clang-tools-extra/trunk/clang-tidy/misc/LambdaFunctionNameCheck.h
>>> Fri Jun  2 13:47:50 2017
>>> @@ -25,7 +25,7 @@ namespace misc {
>>>  class LambdaFunctionNameCheck : public ClangTidyCheck {
>>>  public:
>>>struct SourceRangeLessThan {
>>> -bool operator()(const SourceRange &L, const SourceRange &R) {
>>> +bool operator()(const SourceRange &L, const SourceRange &R) const {
>>>if (L.getBegin() == R.getBegin()) {
>>>  return L.getEnd() < R.getEnd();
>>>}
>>>
>>>
>>> ___
>>> cfe-commits mailing list
>>> cfe-commits@lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>
>>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D32696: More detailed docs for UsingShadowDecl

[Kim Gräsman via cfe-commits]

Ping! Any takers?

Thanks,
- Kim

On Mon, May 1, 2017 at 10:27 AM, Kim Gräsman via Phabricator
 wrote:
> kimgr created this revision.
>
> I couldn't make sense of the docs before, so I sent a question to cfe-dev. 
> Richard was gracious enough to fill in the blanks:
> http://lists.llvm.org/pipermail/cfe-dev/2017-April/053687.html
>
> I've tried to transfer some of his response to the Doxygen for 
> UsingShadowDecl. Here's to hoping I captured everything correctly.
>
>
> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D32696
>
> Files:
>   include/clang/AST/DeclCXX.h
>
>
> Index: include/clang/AST/DeclCXX.h
> ===
> --- include/clang/AST/DeclCXX.h
> +++ include/clang/AST/DeclCXX.h
> @@ -2856,18 +2856,34 @@
>  };
>
>  /// \brief Represents a shadow declaration introduced into a scope by a
> -/// (resolved) using declaration.
> +/// (resolved) using declaration, whose target is made visible for name 
> lookup.
>  ///
>  /// For example,
>  /// \code
>  /// namespace A {
> -///   void foo();
> +///   void foo(int);
> +///   void foo(double);
>  /// }
>  /// namespace B {
>  ///   using A::foo; // <- a UsingDecl
> -/// // Also creates a UsingShadowDecl for A::foo() in B
> +/// // Also creates UsingShadowDecls for both A::foo(int) and
> +/// // A::foo(double) in B
>  /// }
>  /// \endcode
> +///
> +/// Derived class member declarations can hide any shadow declarations 
> brought
> +/// in by a class-scope UsingDecl;
> +/// \code
> +/// class Base {
> +/// public:
> +///   void foo();
> +/// };
> +/// class Derived : public Base {
> +/// public:
> +///   using Base::foo; // <- a UsingDecl
> +///   void foo();  // Hides Base::foo and suppresses its UsingShadowDecl.
> +/// };
> +/// \endcode
>  class UsingShadowDecl : public NamedDecl, public 
> Redeclarable {
>void anchor() override;
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r296193 - [Test] Make Lit tests C++11 compatible #10

[Kim Gräsman via cfe-commits]

Den 25 feb. 2017 7:23 em skrev "Charles Li via cfe-commits" <
cfe-commits@lists.llvm.org>:

Author: lcharles
Date: Fri Feb 24 17:23:53 2017
New Revision: 296193

URL: http://llvm.org/viewvc/llvm-project?rev=296193&view=rev
Log:
[Test] Make Lit tests C++11 compatible #10

Differential Revision: https://reviews.llvm.org/D21626

Modified:
cfe/trunk/test/Modules/Inputs/merge-using-decls/b.h
cfe/trunk/test/Modules/merge-using-decls.cpp
cfe/trunk/test/SemaCXX/PR9572.cpp
cfe/trunk/test/SemaCXX/default-assignment-operator.cpp
cfe/trunk/test/SemaCXX/default-constructor-initializers.cpp
cfe/trunk/test/SemaCXX/warn-thread-safety-parsing.cpp

Modified: cfe/trunk/test/Modules/Inputs/merge-using-decls/b.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/
Modules/Inputs/merge-using-decls/b.h?rev=296193&r1=
296192&r2=296193&view=diff

==
--- cfe/trunk/test/Modules/Inputs/merge-using-decls/b.h (original)
+++ cfe/trunk/test/Modules/Inputs/merge-using-decls/b.h Fri Feb 24 17:23:53
2017
@@ -29,11 +29,13 @@ template struct D : X, T {
   using typename X::t;
 };

+#if __cplusplus <= 199711L // C++11 does not allow access declerations


Typo: declerations, here and throughout.

- Kim
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r292558 - Add documentation for constexpr string builtin support.

[Kim Gräsman via cfe-commits]

Hi Richard,

On Fri, Jan 20, 2017 at 1:58 AM, Richard Smith via cfe-commits
 wrote:
>
> +String builtins
> +---
> +
> +Clang provides constant expression evaluation support for builtins forms of
> +the following functions from the C standard library  header:
> +
> +* ``memchr``
> +* ``memcmp``
> +* ``strchr``

This should say , right?

- Kim
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [clang-tools-extra] r284894 - [Release notes] Mention removed Clang-tidy misc-pointer-and-integral-operation check

[Kim Gräsman via cfe-commits]

Hi Eugene,

On Sat, Oct 22, 2016 at 12:35 AM, Eugene Zelenko via cfe-commits
 wrote:
> Author: eugenezelenko
> Date: Fri Oct 21 17:35:58 2016
> New Revision: 284894
>
> URL: http://llvm.org/viewvc/llvm-project?rev=284894&view=rev
> Log:
> [Release notes] Mention removed Clang-tidy 
> misc-pointer-and-integral-operation check
>
> Modified:
> clang-tools-extra/trunk/docs/ReleaseNotes.rst
>
> Modified: clang-tools-extra/trunk/docs/ReleaseNotes.rst
> URL: 
> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/ReleaseNotes.rst?rev=284894&r1=284893&r2=284894&view=diff
> ==
> --- clang-tools-extra/trunk/docs/ReleaseNotes.rst (original)
> +++ clang-tools-extra/trunk/docs/ReleaseNotes.rst Fri Oct 21 17:35:58 2016
> @@ -73,6 +73,8 @@ Improvements to clang-tidy
>Warns when ``std::move`` is applied to a forwarding reference instead of
>``std::forward``.
>
> +- `misc-pointer-and-integral-operation` check was removed.
> +

Would it be worthwhile to describe *why* it was removed? I haven't
followed the discussions, so this looks a little abrupt.

- Kim
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D23895: [ms] Add support for parsing uuid as a MS attribute

[Kim Gräsman via cfe-commits]

Microsoft used to call their IDL dialect 'MIDL' (which is kind of punny),
don't know if it makes sense to stick with that over 'MSIDL'.

- Kim

Den 26 aug. 2016 4:09 fm skrev "Nico Weber via cfe-commits" <
cfe-commits@lists.llvm.org>:

> thakis updated this revision to Diff 69312.
> thakis marked an inline comment as done.
> thakis added a comment.
>
> comments round 1
>
>
> https://reviews.llvm.org/D23895
>
> Files:
>   include/clang/Basic/Attr.td
>   include/clang/Basic/Attributes.h
>   include/clang/Parse/Parser.h
>   include/clang/Sema/AttributeList.h
>   lib/Parse/ParseDecl.cpp
>   lib/Parse/ParseDeclCXX.cpp
>   lib/Parse/ParseObjc.cpp
>   lib/Parse/ParseOpenMP.cpp
>   lib/Parse/Parser.cpp
>   test/Parser/MicrosoftExtensions.cpp
>   test/Sema/MicrosoftExtensions.c
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D21459: Implement http://wg21.link/P0254R1: "Integrating std::string_view and std::string"

[Kim Gräsman via cfe-commits]

kimgr added a comment.

This is probably not the right place for this discussion, but I thought I'd 
offer one more note.



Comment at: include/string_view:216
@@ +215,3 @@
+   basic_string_view(const _CharT* __s)
+   : __data(__s), __size(_Traits::length(__s)) {}
+

mclow.lists wrote:
> kimgr wrote:
> > mclow.lists wrote:
> > > mclow.lists wrote:
> > > > kimgr wrote:
> > > > > mclow.lists wrote:
> > > > > > kimgr wrote:
> > > > > > > I'm working from the paper at 
> > > > > > > https://isocpp.org/files/papers/N3762.html, and I find it a 
> > > > > > > little sketchy on the policy for nullptrs.
> > > > > > > 
> > > > > > > Since the ctor above accepts nullptr as long as the length is 
> > > > > > > zero, would it make sense to do that here too? That is, only call 
> > > > > > > _Traits::length for non-nullptr __s args?
> > > > > > Reading from N4600: Requires: `[str, str + traits::length(str))` is 
> > > > > > a valid range.
> > > > > > 
> > > > > > So, no - passing `nullptr` here is undefined.
> > > > > > 
> > > > > OK, that feels more principled to me, anyway.
> > > > > 
> > > > > But the `(const char*, size_t)` constructor has the same requirement 
> > > > > and it *does* accept `nullptr`, provided the length is zero. I saw 
> > > > > you had to get rid of the assertion, but I feel like the constructor 
> > > > > should probably not silently accept `nullptr` for zero-sized strings. 
> > > > > Or do you know if that's intentional? Many StringRef/StringPiece 
> > > > > implementations seem to have the same behavior.
> > > > It is absolutely intentional. `[nullptr, nullptr+0)` is a perfectly 
> > > > fine half-open range. It contains no characters.  
> > > > 
> > > > However, the ctor that takes just a pointer has to calculate the length 
> > > > .. by dereferencing the pointer.
> > > > 
> > > > I had to get rid of the assertion because one of the bots (gcc 4.9) has 
> > > > a bug about constexpr ctors in c++11 mode.  Even though the assertion 
> > > > was `#ifdef`ed on C++ > 11, the compiler complained about it.  I'll be 
> > > > putting the assertion back as soon as I can figure out how to keep gcc 
> > > > from complaining.
> > > This was discussed (at length) in LEWG during the design of `string_view`.
> > Ah, got it, thanks! It opens up for cases where `data()` for an empty 
> > `string_view` can sometimes return `""` and sometimes `nullptr`, but I 
> > guess that problem extends beyond `string_view`'s responsibilities.
> > 
> > Thanks for the thorough explanation.
> I think you're laboring under a misapprehension here.
> 
> An empty `string_view` points to *no characters*, not an empty 
> null-terminated string.
> 
> Treating the pointer that you get back from `string_view::data` as a 
> null-terminated string will lead to all sorts of problems.  This is 
> explicitly called out in [string.view.access]/19:
> 
> > Note: Unlike basic_string::data() and string literals, data() may return a 
> > pointer to a buffer that is not null-terminated. Therefore it is typically 
> > a mistake to pass data() to a routine that takes just a const charT* and 
> > expects a null-terminated string.
> 
> 
Thanks, I think I'm free of that particular misapprehension :-)

The case I had in mind was:

void f(const string_view& s) {
  char buf[50] = {0};
  assert(s.size() < 50);
  memcpy(buf, s.data(), s.size());
}

There's no assumption of null-termination here, but as I understand it, 
`memcpy` has undefined behavior if source is `nullptr`, irrespective of whether 
`num` is zero or not.

So these would be valid:

f(string_view("", 0));
f(string_view(""));

but this wouldn't:

f(string_view(nullptr, 0));

which I find slightly asymmetric.

But again, maybe this is not `string_view`'s fault, but `memcpy`'s, in which 
case `nullptr` kludges could be moved to wherever `memcpy` is used instead of 
wherever `string_view::string_view` is used.


https://reviews.llvm.org/D21459



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D21459: Implement http://wg21.link/P0254R1: "Integrating std::string_view and std::string"

[Kim Gräsman via cfe-commits]

kimgr added inline comments.


Comment at: include/string_view:216
@@ +215,3 @@
+   basic_string_view(const _CharT* __s)
+   : __data(__s), __size(_Traits::length(__s)) {}
+

mclow.lists wrote:
> mclow.lists wrote:
> > kimgr wrote:
> > > mclow.lists wrote:
> > > > kimgr wrote:
> > > > > I'm working from the paper at 
> > > > > https://isocpp.org/files/papers/N3762.html, and I find it a little 
> > > > > sketchy on the policy for nullptrs.
> > > > > 
> > > > > Since the ctor above accepts nullptr as long as the length is zero, 
> > > > > would it make sense to do that here too? That is, only call 
> > > > > _Traits::length for non-nullptr __s args?
> > > > Reading from N4600: Requires: `[str, str + traits::length(str))` is a 
> > > > valid range.
> > > > 
> > > > So, no - passing `nullptr` here is undefined.
> > > > 
> > > OK, that feels more principled to me, anyway.
> > > 
> > > But the `(const char*, size_t)` constructor has the same requirement and 
> > > it *does* accept `nullptr`, provided the length is zero. I saw you had to 
> > > get rid of the assertion, but I feel like the constructor should probably 
> > > not silently accept `nullptr` for zero-sized strings. Or do you know if 
> > > that's intentional? Many StringRef/StringPiece implementations seem to 
> > > have the same behavior.
> > It is absolutely intentional. `[nullptr, nullptr+0)` is a perfectly fine 
> > half-open range. It contains no characters.  
> > 
> > However, the ctor that takes just a pointer has to calculate the length .. 
> > by dereferencing the pointer.
> > 
> > I had to get rid of the assertion because one of the bots (gcc 4.9) has a 
> > bug about constexpr ctors in c++11 mode.  Even though the assertion was 
> > `#ifdef`ed on C++ > 11, the compiler complained about it.  I'll be putting 
> > the assertion back as soon as I can figure out how to keep gcc from 
> > complaining.
> This was discussed (at length) in LEWG during the design of `string_view`.
Ah, got it, thanks! It opens up for cases where `data()` for an empty 
`string_view` can sometimes return `""` and sometimes `nullptr`, but I guess 
that problem extends beyond `string_view`'s responsibilities.

Thanks for the thorough explanation.


https://reviews.llvm.org/D21459



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D21459: Implement http://wg21.link/P0254R1: "Integrating std::string_view and std::string"

[Kim Gräsman via cfe-commits]

kimgr added inline comments.


Comment at: include/string_view:216
@@ +215,3 @@
+   basic_string_view(const _CharT* __s)
+   : __data(__s), __size(_Traits::length(__s)) {}
+

mclow.lists wrote:
> kimgr wrote:
> > I'm working from the paper at https://isocpp.org/files/papers/N3762.html, 
> > and I find it a little sketchy on the policy for nullptrs.
> > 
> > Since the ctor above accepts nullptr as long as the length is zero, would 
> > it make sense to do that here too? That is, only call _Traits::length for 
> > non-nullptr __s args?
> Reading from N4600: Requires: `[str, str + traits::length(str))` is a valid 
> range.
> 
> So, no - passing `nullptr` here is undefined.
> 
OK, that feels more principled to me, anyway.

But the `(const char*, size_t)` constructor has the same requirement and it 
*does* accept `nullptr`, provided the length is zero. I saw you had to get rid 
of the assertion, but I feel like the constructor should probably not silently 
accept `nullptr` for zero-sized strings. Or do you know if that's intentional? 
Many StringRef/StringPiece implementations seem to have the same behavior.


https://reviews.llvm.org/D21459



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D21459: Implement http://wg21.link/P0254R1: "Integrating std::string_view and std::string"

[Kim Gräsman via cfe-commits]

kimgr added a subscriber: kimgr.
kimgr added a comment.

Inline question on ctor+nullptr



Comment at: include/string_view:216
@@ +215,3 @@
+   basic_string_view(const _CharT* __s)
+   : __data(__s), __size(_Traits::length(__s)) {}
+

I'm working from the paper at https://isocpp.org/files/papers/N3762.html, and I 
find it a little sketchy on the policy for nullptrs.

Since the ctor above accepts nullptr as long as the length is zero, would it 
make sense to do that here too? That is, only call _Traits::length for 
non-nullptr __s args?


https://reviews.llvm.org/D21459



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D22087: [clang-rename] add basic vim integration

[Kim Gräsman via cfe-commits]

kimgr added a subscriber: kimgr.
kimgr added a comment.

I only caught this typo after it was committed.



Comment at: clang-tools-extra/trunk/clang-rename/tool/clang-rename.py:17-18
@@ +16,4 @@
+All you have to do now is to place a cursor on a variable/function/class which
+you would like to rename and press ',cr'. You will be promted a new name if the
+cursor points to a valid symbol.
+'''

s/promted/prompted for/


Repository:
  rL LLVM

http://reviews.llvm.org/D22087



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r272247 - [Sema] Don't crash when a field w/ a mem-initializer clashes with a record name

[Kim Gräsman via cfe-commits]

On Thu, Jun 9, 2016 at 4:51 PM, David Majnemer  wrote:
> It would mean that the instantiation of the class template gained a field
> which should be impossible.

OK, so assert(Lookup.size() > 0) always holds? Makes sense, thanks.

- Kim
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r272247 - [Sema] Don't crash when a field w/ a mem-initializer clashes with a record name

[Kim Gräsman via cfe-commits]

On Thu, Jun 9, 2016 at 7:26 AM, David Majnemer via cfe-commits
 wrote:
> Author: majnemer
> Date: Thu Jun  9 00:26:56 2016
> New Revision: 272247
>
> URL: http://llvm.org/viewvc/llvm-project?rev=272247&view=rev
> Log:
> [Sema] Don't crash when a field w/ a mem-initializer clashes with a record 
> name
>
> It is possible for a field and a class to have the same name.  In such
> cases, performing lookup for the field might return a result set with
> more than one entry.  An overzealous assertion fired, causing us to
> crash instead of using the non-class lookup result.
>
> This fixes PR28060.
>
> Modified:
> cfe/trunk/lib/Sema/SemaTemplateInstantiate.cpp
> cfe/trunk/test/SemaCXX/member-init.cpp
>
> Modified: cfe/trunk/lib/Sema/SemaTemplateInstantiate.cpp
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaTemplateInstantiate.cpp?rev=272247&r1=272246&r2=272247&view=diff
> ==
> --- cfe/trunk/lib/Sema/SemaTemplateInstantiate.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaTemplateInstantiate.cpp Thu Jun  9 00:26:56 2016
> @@ -2637,8 +2637,7 @@ Sema::InstantiateClassMembers(SourceLoca
>  Instantiation->getTemplateInstantiationPattern();
>  DeclContext::lookup_result Lookup =
>  ClassPattern->lookup(Field->getDeclName());
> -assert(Lookup.size() == 1);
> -FieldDecl *Pattern = cast(Lookup[0]);
> +FieldDecl *Pattern = cast(Lookup.front());
>  InstantiateInClassInitializer(PointOfInstantiation, Field, Pattern,
>TemplateArgs);
>}

Now what if there is no match? Or is that guaranteed (given the prior assert)?

- Kim
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D19816: [find-all-symbols] Add IWYU private pragma support.

[Kim Gräsman via cfe-commits]

Re semantics, you may want to link to IWYU's docs at
https://github.com/include-what-you-use/include-what-you-use/blob/master/docs/IWYUPragmas.md
.

- Kim
Den 3 maj 2016 6:34 em skrev "Manuel Klimek via cfe-commits" <
cfe-commits@lists.llvm.org>:

> klimek added inline comments.
>
> 
> Comment at: include-fixer/find-all-symbols/FindAllSymbols.cpp:173-182
> @@ -165,1 +172,12 @@
>
> +void FindAllSymbols::addPragmaHeader(FileID ID, llvm::StringRef FilePath)
> {
> +  PragmaHeaderMap[ID] = FilePath;
> +}
> +
> +llvm::Optional FindAllSymbols::getPragmaHeader(FileID ID)
> const {
> +  auto It = PragmaHeaderMap.find(ID);
> +  if (It == PragmaHeaderMap.end())
> +return llvm::None;
> +  return It->second;
> +}
> +
> 
> It seems weird to add this and just forward the interface.
>
> 
> Comment at: include-fixer/find-all-symbols/FindAllSymbols.h:49
> @@ -45,1 +48,3 @@
>
> +  void addPragmaHeader(FileID ID, llvm::StringRef FilePath);
> +
> 
> I think the fact that those are generated via IWYU comments are an
> implementation detail, and this code doesn't care. Perhaps call it
> HeaderMapping or HeaderRemapping. Also, it's unclear what the semantics
> are, so I think this needs a comment.
>
> 
> Comment at: include-fixer/find-all-symbols/PragmaCommentHandler.h:23
> @@ +22,3 @@
> +public:
> +  PragmaCommentHandler(FindAllSymbols *Matcher) : Matcher(Matcher) {}
> +
> 
> I'd pull out an interface like HeaderMapCollector or
> ForwardingHeaderCollector, or even just pass in a std::function that
> collects the header. Or, just make this PragmaCommentHandler have a method
> to return a list of forwarding headers?
> Generally, I think this couples the two classes much more than necessary.
>
>
> Repository:
>   rL LLVM
>
> http://reviews.llvm.org/D19816
>
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D19816: [find-all-symbols] Add IWYU private pragma support.

[Kim Gräsman via cfe-commits]

kimgr added a subscriber: kimgr.
kimgr added a comment.

Re semantics, you may want to link to IWYU's docs at
https://github.com/include-what-you-use/include-what-you-use/blob/master/docs/IWYUPragmas.md
.

- Kim

Den 3 maj 2016 6:34 em skrev "Manuel Klimek via cfe-commits" <
cfe-commits@lists.llvm.org>:

> klimek added inline comments.

> 

> 

>  Comment at: include-fixer/find-all-symbols/FindAllSymbols.cpp:173-182

>  @@ -165,1 +172,12 @@

> 

> +void FindAllSymbols::addPragmaHeader(FileID ID, llvm::StringRef FilePath)

>  {

>  +  PragmaHeaderMap[ID] = FilePath;

>  +}

>  +

>  +llvm::Optional FindAllSymbols::getPragmaHeader(FileID ID)

>  const {

>  +  auto It = PragmaHeaderMap.find(ID);

>  +  if (It == PragmaHeaderMap.end())

>  +return llvm::None;

>  +  return It->second;

>  +}

>  +

> 

>  

> 

> It seems weird to add this and just forward the interface.

> 

> 

>  Comment at: include-fixer/find-all-symbols/FindAllSymbols.h:49

>  @@ -45,1 +48,3 @@

> 

> +  void addPragmaHeader(FileID ID, llvm::StringRef FilePath);

>  +

> 

>  

> 

> I think the fact that those are generated via IWYU comments are an

>  implementation detail, and this code doesn't care. Perhaps call it

>  HeaderMapping or HeaderRemapping. Also, it's unclear what the semantics

>  are, so I think this needs a comment.

> 

> 

>  Comment at: include-fixer/find-all-symbols/PragmaCommentHandler.h:23

>  @@ +22,3 @@

>  +public:

>  +  PragmaCommentHandler(FindAllSymbols *Matcher) : Matcher(Matcher) {}

>  +

> 

>  

> 

> I'd pull out an interface like HeaderMapCollector or

>  ForwardingHeaderCollector, or even just pass in a std::function that

>  collects the header. Or, just make this PragmaCommentHandler have a method

>  to return a list of forwarding headers?

>  Generally, I think this couples the two classes much more than necessary.

> 

> Repository:

> 

>   rL LLVM

> 

> http://reviews.llvm.org/D19816

> 

>  ___

> 

> cfe-commits mailing list

>  cfe-commits@lists.llvm.org

>  http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits



Repository:
  rL LLVM

http://reviews.llvm.org/D19816



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D18852: [clang-tidy] fix a crash with -fdelayed-template-parsing in UnnecessaryValueParamCheck.

[Kim Gräsman via cfe-commits]

kimgr added inline comments.


Comment at: test/clang-tidy/performance-unnecessary-value-param-delayed.cpp:1
@@ +1,2 @@
+// RUN: %check_clang_tidy %s performance-unnecessary-value-param %t -- -- 
-fdelayed-template-parsing
+

etienneb wrote:
> kimgr wrote:
> > Are the double `--` necessary?
> Yes, this is fine. See other *delayed.cpp test.
> 
> Example:
> 
> // RUN: %check_clang_tidy %s cppcoreguidelines-pro-type-member-init %t -- -- 
> -fdelayed-template-parsing
> 
Oh, I see now that it's a peculiarity of `check_clang_tidy.py`. Sorry for the 
noise!


http://reviews.llvm.org/D18852



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D18852: [clang-tidy] fix a crash with -fdelayed-template-parsing in UnnecessaryValueParamCheck.

[Kim Gräsman via cfe-commits]

kimgr added a comment.

The RUN line looks weird to me, but I'm no lit expert...



Comment at: test/clang-tidy/performance-unnecessary-value-param-delayed.cpp:1
@@ +1,2 @@
+// RUN: %check_clang_tidy %s performance-unnecessary-value-param %t -- -- 
-fdelayed-template-parsing
+

Are the double `--` necessary?


http://reviews.llvm.org/D18852



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D18852: [clang-tidy] fix a crash with -fdelayed-template-parsing in UnnecessaryValueParamCheck.

[Kim Gräsman via cfe-commits]

kimgr added a subscriber: kimgr.
kimgr added a comment.

A test case would be nice here. You can repro on all systems by passing 
`-fdelayed-template-parsing` explicitly.


http://reviews.llvm.org/D18852



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D16259: Add clang-tidy readability-redundant-control-flow check

[Kim Gräsman via cfe-commits]

On Mon, Feb 1, 2016 at 4:32 PM, Aaron Ballman  wrote:
>
> 
> Comment at: clang-tidy/readability/RedundantControlFlowCheck.cpp:60
> @@ +59,3 @@
> +  if (const auto *Return = dyn_cast(*last))
> +issueDiagnostic(Result, Block, Return->getSourceRange(),
> +RedundantReturnDiag);
> 
> It is the LLVM coding style (though I don't personally care for it), and you 
> are right -- a clang-tidy check in the LLVM module wouldn't be amiss. :-)

Isn't this: 
http://clang.llvm.org/extra/clang-tidy/checks/readability-braces-around-statements.html?

- Kim
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r258768 - [Sema] Incomplete types are OK for covariant returns

[Kim Gräsman via cfe-commits]

On Sun, Jan 31, 2016 at 5:24 PM, Kim Gräsman  wrote:
> On Sun, Jan 31, 2016 at 2:50 PM, David Majnemer
>  wrote:
>>
>> It is the same issue as CWG defect report 1250:
>> http://wg21.cmeerw.net/cwg/issue1250
>>
>> I forget how to tell how far back a DR applies but I'd guess this one goes
>> as far back as C++11.
>
> Thanks, I didn't know there was a DR for this.

Oh, this should probably be updated:
http://clang.llvm.org/cxx_dr_status.html#1250

- K
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r258768 - [Sema] Incomplete types are OK for covariant returns

[Kim Gräsman via cfe-commits]

On Sun, Jan 31, 2016 at 2:50 PM, David Majnemer
 wrote:
>
> It is the same issue as CWG defect report 1250:
> http://wg21.cmeerw.net/cwg/issue1250
>
> I forget how to tell how far back a DR applies but I'd guess this one goes
> as far back as C++11.

Thanks, I didn't know there was a DR for this.

I find the standards text is a little vague but your change seems to
indicate that cv-qualifiers never weighs into covariance.

The reason I ask is we handle this explicitly in IWYU, and we treat
cv-qual differences as covariant (requiring the complete type). It
doesn't look like that was the standard's intent. We should probably
follow Clang here in a coming release, assuming other compilers agree.
It looks from the bug-report that they do, at least newer versions.

Thanks,
- Kim
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r258768 - [Sema] Incomplete types are OK for covariant returns

[Kim Gräsman via cfe-commits]

Hi David,

Should this be guarded by if(cxx14)? I think the complete type was required
by earlier standards.

- Kim
Den 26 jan 2016 2:40 fm skrev "David Majnemer via cfe-commits" <
cfe-commits@lists.llvm.org>:

> Author: majnemer
> Date: Mon Jan 25 19:37:01 2016
> New Revision: 258768
>
> URL: http://llvm.org/viewvc/llvm-project?rev=258768&view=rev
> Log:
> [Sema] Incomplete types are OK for covariant returns
>
> Per C++14 [class.virtual]p8, it is OK for the return type's class type
> to be incomplete so long as the return type is the same between the base
> and complete classes.
>
> This fixes PR26297.
>
> Modified:
> cfe/trunk/lib/Sema/SemaDeclCXX.cpp
> cfe/trunk/test/SemaCXX/virtual-override.cpp
>
> Modified: cfe/trunk/lib/Sema/SemaDeclCXX.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclCXX.cpp?rev=258768&r1=258767&r2=258768&view=diff
>
> ==
> --- cfe/trunk/lib/Sema/SemaDeclCXX.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaDeclCXX.cpp Mon Jan 25 19:37:01 2016
> @@ -13020,19 +13020,20 @@ bool Sema::CheckOverridingFunctionReturn
>  return true;
>}
>
> -  // C++ [class.virtual]p6:
> -  //   If the return type of D::f differs from the return type of B::f,
> the
> -  //   class type in the return type of D::f shall be complete at the
> point of
> -  //   declaration of D::f or shall be the class type D.
> -  if (const RecordType *RT = NewClassTy->getAs()) {
> -if (!RT->isBeingDefined() &&
> -RequireCompleteType(New->getLocation(), NewClassTy,
> -diag::err_covariant_return_incomplete,
> -New->getDeclName()))
> -return true;
> -  }
> -
>if (!Context.hasSameUnqualifiedType(NewClassTy, OldClassTy)) {
> +// C++14 [class.virtual]p8:
> +//   If the class type in the covariant return type of D::f differs
> from
> +//   that of B::f, the class type in the return type of D::f shall be
> +//   complete at the point of declaration of D::f or shall be the
> class
> +//   type D.
> +if (const RecordType *RT = NewClassTy->getAs()) {
> +  if (!RT->isBeingDefined() &&
> +  RequireCompleteType(New->getLocation(), NewClassTy,
> +  diag::err_covariant_return_incomplete,
> +  New->getDeclName()))
> +return true;
> +}
> +
>  // Check if the new class derives from the old class.
>  if (!IsDerivedFrom(New->getLocation(), NewClassTy, OldClassTy)) {
>Diag(New->getLocation(), diag::err_covariant_return_not_derived)
>
> Modified: cfe/trunk/test/SemaCXX/virtual-override.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/virtual-override.cpp?rev=258768&r1=258767&r2=258768&view=diff
>
> ==
> --- cfe/trunk/test/SemaCXX/virtual-override.cpp (original)
> +++ cfe/trunk/test/SemaCXX/virtual-override.cpp Mon Jan 25 19:37:01 2016
> @@ -289,3 +289,15 @@ namespace PR8168 {
>  static void foo() {} // expected-error{{'static' member function
> 'foo' overrides a virtual function}}
>};
>  }
> +
> +namespace PR26297 {
> +struct Incomplete;
> +
> +struct Base {
> +  virtual const Incomplete *meow() = 0;
> +};
> +
> +struct Derived : Base {
> +  virtual Incomplete *meow() override { return nullptr; }
> +};
> +}
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D16535: [clang-tidy] Check to find unintended semicolons that changes the semantics.

[Kim Gräsman via cfe-commits]

kimgr added a subscriber: kimgr.
kimgr added a comment.

Cool check, I like how it pays attention to indentation!

I found some minor doc nits, see inline.



Comment at: docs/clang-tidy/checks/misc-suspicious-semicolon.rst:8-9
@@ +7,4 @@
+the code. More specifically, it looks for `if`, `while`, `for` and `for-range`
+statements whose body is a single semicolon, and then analyzes the
+context of the code (e.g. indentation) in an attempt to determine whether that
+is intentional.

"context" looks like it would fit on the wrapped line.


Comment at: docs/clang-tidy/checks/misc-suspicious-semicolon.rst:20
@@ +19,3 @@
+Here the body of the `if` statement consists of only the semicolon at the end 
of
+the first line, and `x` will be increased regardless of the condition.
+

incremented?


Comment at: docs/clang-tidy/checks/misc-suspicious-semicolon.rst:29
@@ +28,3 @@
+As a result of this code, `processLine()` will only be called once, when the
+`while` loop with the empty body exits with `line == NULL`. The identation of
+the code indicates the intention of the programmer.

Typo: identation


Comment at: docs/clang-tidy/checks/misc-suspicious-semicolon.rst:71
@@ +70,3 @@
+
+In this case the checker will assume that you know what you are doing, and will
+not raise a warning.

There's been some preference for "check" over "checker" lately.


Comment at: test/clang-tidy/misc-suspicious-semicolon.cpp:88
@@ +87,3 @@
+  char c = 'b';
+  char * s = "a";
+  if (s == "(" || s != "'" || c == '"') {

Weird pointer alignment is a little distracting here, better stick with LLVM 
convention and attach it to `s`?


http://reviews.llvm.org/D16535



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D16259: Add clang-tidy readability-redundant-return check

[Kim Gräsman via cfe-commits]

kimgr added inline comments.


Comment at: test/clang-tidy/readability-redundant-return.cpp:21-24
@@ +20,6 @@
+
+void g(int i) {
+  if (i < 0) {
+return;
+  }
+  if (i < 10) {

LegalizeAdulthood wrote:
> kimgr wrote:
> > What happens to guard clauses invoking void functions?
> > 
> > void h() {
> > }
> > 
> > void g(int i) {
> >   if(i < 0) {
> > return h();
> >   }
> > }
> > 
> Nothing because the last statement of the `compoundStmt` that is the function 
> body is an `if` statement and not a `return` statement.
> 
> That is exactly why lines 21-24 are in the test suite :).
Ah, I hadn't understood the mechanics of the check. I read the docs, and now I 
do! Don't mind me :-)


http://reviews.llvm.org/D16259



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D16259: Add clang-tidy readability-redundant-return check

[Kim Gräsman via cfe-commits]

kimgr added a subscriber: kimgr.
kimgr added a comment.

Came up with another test case.



Comment at: test/clang-tidy/readability-redundant-return.cpp:21-24
@@ +20,6 @@
+
+void g(int i) {
+  if (i < 0) {
+return;
+  }
+  if (i < 10) {

What happens to guard clauses invoking void functions?

void h() {
}

void g(int i) {
  if(i < 0) {
return h();
  }
}



http://reviews.llvm.org/D16259



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D9600: Add scan-build python implementation

[Kim Gräsman via cfe-commits]

kimgr added a subscriber: kimgr.
kimgr added a comment.

> 

>  Comment at: tools/scan-build-py/bin/analyze-cc:14

>  @@ +13,2 @@

>  +from libscanbuild.analyze import wrapper

>  +sys.exit(wrapper(False))

> 

>  

> 

> It is hard to figure out/search which functions actually get called from the 
> top level tools. Could you rename all of the public functions so that they

>  have unique names? For example, "wrapper" -> "scan_build_wrapper". This 
> would greatly improve searchability!


A nice pattern in Python is to avoid importing individual
functions/classes, and instead use the module name as a namespace,
e.g.

  from libscanbuild import analyze
  sys.exit(analyze.wrapper(False))

I don't know if that addresses your concern? I haven't looked at the
code in more detail.

- Kim


http://reviews.llvm.org/D9600



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12407: [clang-format-vs] Add an option to reformat source code when file is saved to disk

[Kim Gräsman via cfe-commits]

kimgr added a subscriber: kimgr.
kimgr added a comment.

Add debugging ideas.



Comment at: tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs:86
@@ -69,1 +85,3 @@
 
+IComponentModel componentModel = 
GetService(typeof(SComponentModel)) as IComponentModel;
+editorAdaptersFactoryService = 
componentModel.GetService();

aaron.ballman wrote:
> hans wrote:
> > aaron.ballman wrote:
> > > berenm wrote:
> > > > I did more tests on my side, and apparently this line does not work on 
> > > > VS2012, componentModel is null. I don't know at all why and how to fix 
> > > > it, and it works fine starting with VS2013.
> > > Our minimum supported MSVC version for development is 2013. Do we 
> > > document supported versions for clang-format? Do we want to support 
> > > versions older than the development version we're on?
> > The manifest claims support for 2010 and later. I usually test with 2012 
> > when I build the weekly snapshot.
> > 
> > Our clang-cl VS integration also tries to support 2010 and later, so I 
> > think it would be nice if the clang-format plugin does too.
> Thank you for the information! That sounds good to me. Then I think berenm's 
> issue should be resolved if possible.
I haven't had time to dig into this too much, but here's an idea:

You're only getting the IComponentModel to get the 
IVsEditorAdaptersFactoryService, which in turn is only used to get at the 
document's text buffer in OnBeforeSave.

It seems to me there *should* be a more straightforward way to do that without 
involving COM interop. Unfortunately, I don't know what it is :-)

Google around, and this article describes something similar:
http://schmalls.com/2015/01/19/adventures-in-visual-studio-extension-development-part-2

I don't know if maybe you have the wrong RunningDocumentTable service, it seems 
very COM-styled, compared to the one used in the article.


http://reviews.llvm.org/D12407



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r248984 - Test fix

[Kim Gräsman via cfe-commits]

On Fri, Oct 2, 2015 at 3:09 AM, Richard Smith via cfe-commits
 wrote:
> On Thu, Oct 1, 2015 at 6:01 AM, Renato Golin via cfe-commits
>  wrote:
>>
>> Right, I reverted both commits on r249005. Please, let me know if you
>> need help testing on ARM before the next commit. This looks like it
>> could be tested on any 32-bit platform, though, so you should be able
>> to get it passing on ARM if you test and make it pass on x86.
>
>
> It looks like it's just failing because there's more than 10 metadata nodes
> in this test for your bot.
>
> +// CHECK-INVARIANT: load {{.*}} !invariant.load ![[EMPTY_NODE:[0-9]]]
>
> ... should be
>
> +// CHECK-INVARIANT: load {{.*}} !invariant.load ![[EMPTY_NODE:[0-9]*]]

Drive-by review: shouldn't that be [0-9]+ -- you require at least one, right?

- Kim
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12759: [clang-tidy] Add misc-sizeof-container check to find sizeof() uses on stlcontainers.

[Kim Gräsman via cfe-commits]

Late to the party, but I wanted to ask: is there a way to indicate to
the checker that we really *did* mean sizeof()?

I think I've stumbled over code in our code base that uses
sizeof(container) to report memory usage statistics and it seems
valid, so it'd be nice if this checker could be silenced on a
case-by-case basis.

Thanks,
- Kim

On Sat, Sep 12, 2015 at 12:09 AM, Alexander Kornienko via cfe-commits
 wrote:
> Indeed. But this has been fixed before I could get to it.
>
>
> On Thu, Sep 10, 2015 at 10:47 PM, Aaron Ballman via cfe-commits
>  wrote:
>>
>> aaron.ballman added a comment.
>>
>> This appears to have broken one of the bots:
>>
>> http://bb.pgr.jp/builders/ninja-x64-msvc-RA-centos6/builds/15065
>>
>>
>> http://reviews.llvm.org/D12759
>>
>>
>>
>> ___
>> cfe-commits mailing list
>> cfe-commits@lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D10431: PR21174 - clang only searches current working directory for precompiled include file

[Kim Gräsman via cfe-commits]

On Tue, Aug 18, 2015 at 2:00 AM, Richard Smith  wrote:
>
>> > Should this logic for PCH/PTH scanning move out of the driver and into
>> > the frontend?
>
>> gcc also checks if the gch file is a directory and if so finds the first
>> file with matching language / defines etc. It also uses gch files for
>> #included files instead of just -included files
>> (https://gcc.gnu.org/onlinedocs/gcc/Precompiled-Headers.html). I thought
>> clang intentionally did none of this so it doesn't have to stat all these
>> directories.
>
> Right, so the question is: what do we want our semantics to be here? Do we
> have sufficient motivation to change our existing design to something closer
> to GCC's? Or should we be encouraging people to use modules instead of PCH
> here, which has all of the desirable properties of the above, and doesn't
> have the "no preceding tokens" limitation?

We find Clang's GCC compat very useful overall, because we use GCC as
a cross-compiler and we can more or less drop in Clang for better
diagnostics and tooling.

So from a personal convenience standpoint, Nikola's patch would be
sufficient, because it would bring Clang just close enough to GCC to
solve our problem (we've now worked around it by disabling PCH in our
build system when we build for Clang.)

Encouraging modules would (I suppose?) create a split where Clang had
modules and other compilers had precompiled headers. For a Clang-only
shop, that's fine, but for a GCC/MSVC/Clang environment like ours,
that would be more work to find common ground. The remaining GCC PCH
behavior sounds mostly esoteric, so I, personally, don't see any point
in trying to mimic it, but others will probably disagree here.

All that said, I think we can solve this locally by just changing our
build system to -include an absolute path, so this isn't critical for
us at the moment. It would be convenient for us if this compat glitch
could be solved, but if you'd rather focus on modules, I can
understand that too :-)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D10431: PR21174 - clang only searches current working directory for precompiled include file

[Kim Gräsman via cfe-commits]

kimgr added inline comments.


Comment at: lib/Driver/Tools.cpp:398
@@ +397,3 @@
+  FoundPTH = !UsePCH;
+}
+  }

kimgr wrote:
> kimgr wrote:
> > kimgr wrote:
> > > Add a `break;` here so we don't continue searching after a valid path has 
> > > been found
> > The GCC docs here [1] say:
> > 
> > If not found there, it is searched for in the remainder of the #include 
> > "..." search chain as normal.
> > 
> > I can't tell if the quotes are significant and if they mean only -I is 
> > searched. I don't have a GCC environment currently to test with.
> > 
> > [1] 
> > https://gcc.gnu.org/onlinedocs/gcc-4.9.3/gcc/Preprocessor-Options.html#index-nostdinc_002b_002b-1026
> I found a FreeBSD machine and set up GCC. I had to adjust the repro case to 
> use `truss` instead of `strace`, so I hope I didn't mess anything up.
> 
> I removed the rule generating the .gch file, so GCC would have to keep 
> searching and it appears to be following the entire include search path, 
> including system paths. Trace below:
> 
> ...
> 30860: stat("./precompiled.header.gch",0x7fffe540) ERR#2 'No such file or 
> directory'
> 30860: open("./precompiled.header",O_NOCTTY,0666) ERR#2 'No such file or 
> directory'
> 30860: stat("./build/include/precompiled.header.gch",0x7fffe540) ERR#2 
> 'No such file or directory'
> 30860: open("./build/include/precompiled.header",O_NOCTTY,0666) ERR#2 'No 
> such file or directory'
> 30860: stat("./include/precompiled.header.gch",0x7fffe540) ERR#2 'No such 
> file or directory'
> 30860: open("./include/precompiled.header",O_NOCTTY,0666) ERR#2 'No such file 
> or directory'
> 30860: lstat("/usr",{ mode=drwxr-xr-x ,inode=9,size=16,blksize=4096 }) = 0 
> (0x0)
> 30860: lstat("/usr/local",{ mode=drwxr-xr-x ,inode=115,size=16,blksize=4096 
> }) = 0 (0x0)
> 30860: lstat("/usr/local/lib",{ mode=drwxr-xr-x 
> ,inode=16476,size=1318,blksize=84480 }) = 0 (0x0)
> 30860: lstat("/usr/local/lib/gcc48",{ mode=drwxr-xr-x 
> ,inode=24335,size=66,blksize=131072 }) = 0 (0x0)
> 30860: lstat("/usr/local/lib/gcc48/include",{ mode=drwxr-xr-x 
> ,inode=24336,size=3,blksize=131072 }) = 0 (0x0)
> 30860: lstat("/usr/local/lib/gcc48/include/c++",{ mode=drwxr-xr-x 
> ,inode=24337,size=98,blksize=131072 }) = 0 (0x0)
> 30860: 
> lstat("/usr/local/lib/gcc48/include/c++/precompiled.header",0x7fffd3d8) 
> ERR#2 'No such file or directory'
> 30860: 
> stat("/usr/local/lib/gcc48/include/c++/precompiled.header.gch",0x7fffe540)
>  ERR#2 'No such file or directory'
> 30860: 
> open("/usr/local/lib/gcc48/include/c++/precompiled.header",O_NOCTTY,0666) 
> ERR#2 'No such file or directory'
> 30860: lstat("/usr",{ mode=drwxr-xr-x ,inode=9,size=16,blksize=4096 }) = 0 
> (0x0)
> 30860: lstat("/usr/local",{ mode=drwxr-xr-x ,inode=115,size=16,blksize=4096 
> }) = 0 (0x0)
> 30860: lstat("/usr/local/lib",{ mode=drwxr-xr-x 
> ,inode=16476,size=1318,blksize=84480 }) = 0 (0x0)
> 30860: lstat("/usr/local/lib/gcc48",{ mode=drwxr-xr-x 
> ,inode=24335,size=66,blksize=131072 }) = 0 (0x0)
> 30860: lstat("/usr/local/lib/gcc48/include",{ mode=drwxr-xr-x 
> ,inode=24336,size=3,blksize=131072 }) = 0 (0x0)
> 30860: lstat("/usr/local/lib/gcc48/include/c++",{ mode=drwxr-xr-x 
> ,inode=24337,size=98,blksize=131072 }) = 0 (0x0)
> 30860: lstat("/usr/local/lib/gcc48/include/c++/x86_64-portbld-freebsd10.1",{ 
> mode=drwxr-xr-x ,inode=21305,size=4,blksize=131072 }) = 0 (0x0)
> 30860: 
> lstat("/usr/local/lib/gcc48/include/c++/x86_64-portbld-freebsd10.1/precompiled.header",0x7fffd3d8)
>  ERR#2 'No such file or directory'
> 30860: 
> stat("/usr/local/lib/gcc48/include/c++//x86_64-portbld-freebsd10.1/precompiled.header.gch",0x7fffe540)
>  ERR#2 'No such file or directory'
> 30860: 
> open("/usr/local/lib/gcc48/include/c++//x86_64-portbld-freebsd10.1/precompiled.header",O_NOCTTY,0666)
>  ERR#2 'No such file or directory'
> 30860: lstat("/usr",{ mode=drwxr-xr-x ,inode=9,size=16,blksize=4096 }) = 0 
> (0x0)
> 30860: lstat("/usr/local",{ mode=drwxr-xr-x ,inode=115,size=16,blksize=4096 
> }) = 0 (0x0)
> 30860: lstat("/usr/local/lib",{ mode=drwxr-xr-x 
> ,inode=16476,size=1318,blksize=84480 }) = 0 (0x0)
> 30860: lstat("/usr/local/lib/gcc48",{ mode=drwxr-xr-x 
> ,inode=24335,size=66,blksize=131072 }) = 0 (0x0)
> 30860: lstat("/usr/local/lib/gcc48/include",{ mode=drwxr-xr-x 
> ,inode=24336,size=3,blksize=131072 }) = 0 (0x0)
> 30860: lstat("/usr/local/lib/gcc48/include/c++",{ mode=drwxr-xr-x 
> ,inode=24337,size=98,blksize=131072 }) = 0 (0x0)
> 30860: lstat("/usr/local/lib/gcc48/include/c++/backward",{ mode=drwxr-xr-x 
> ,inode=21290,size=10,blksize=131072 }) = 0 (0x0)
> 30860: 
> lstat("/usr/local/lib/gcc48/include/c++/backward/precompiled.header",0x7fffd3d8)
>  ERR#2 'No such file or directory'
> 30860: 
> stat("/usr/local/lib/gcc48/include/c++//backward/precompiled.header.gch",0x7fffe540)
>  ERR#2 'No such file or directory'
> 30860: 
> open("/usr/local/lib/gcc48/include/c++//ba