[PATCH] D60539: Add -std=c++14 language standard option to tests that require C++14 default

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D60539#1469012 , @nemanjai wrote:

> > Do you need to build clangd? We explicitly don't aim to support building 
> > everywhere clang can be built, maybe we should just disable in this case?
>
> Our environment includes various OS levels running on PowerPC. We certainly 
> wouldn't want to disable building/testing `clangd` on all our PowerPC 
> machines. Is there a way to disable it only on certain OS levels?
>
> Furthermore, it seems a little too intrusive to disable an otherwise 
> functional component simply because some test cases rely on a specific 
> language standard default.
>
> Would it be an acceptable solution to add another `StringRef` parameter to 
> `ShouldCollectSymbolTest::build()` - let's call it `ExtraArgs`, to which we 
> can add options such as `-std=c++14` if the test being built relies on that 
> option?


My concern is

- a large fraction of our tests, not just those in this file. rely on the 
default std version (I suspect setting it to c++98 will reveal that). I don't 
think maintaining this information alongside each test and plumbing it through 
every test helper is reasonable. If we want to be robust to changes in this 
flag, I think need to make at least TestTU do the right thing by default. 
Unfortunately the most obvious way to do that (adding `-std-default`) won't 
work.
- there's no buildbot coverage of these configurations, so I'm not sure how 
we'll keep them clean.

Given there's no obvious alternative and the patch is small, it seems OK to 
land this (with a suitable comment), but it's hard for us to commit to 
maintaining it e.g.

- if we add non-C++ testcases in SymbolCollectorTests and need to remove the arg
- for other values of `CLANG_DEFAULT_STD_CXX`
- as more tests are added in the future


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60539/new/

https://reviews.llvm.org/D60539



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

[PATCH] D60539: Add -std=c++14 language standard option to tests that require C++14 default

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added a comment.

> Do you need to build clangd? We explicitly don't aim to support building 
> everywhere clang can be built, maybe we should just disable in this case?

Our environment includes various OS levels running on PowerPC. We certainly 
wouldn't want to disable building/testing `clangd` on all our PowerPC machines. 
Is there a way to disable it only on certain OS levels?

Furthermore, it seems a little too intrusive to disable an otherwise functional 
component simply because some test cases rely on a specific language standard 
default.

Would it be an acceptable solution to add another `StringRef` parameter to 
`ShouldCollectSymbolTest::build()` - let's call it `ExtraArgs`, to which we can 
add options such as `-std=c++14` if the test being built relies on that option?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60539/new/

https://reviews.llvm.org/D60539



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

[PATCH] D60539: Add -std=c++14 language standard option to tests that require C++14 default

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D60539#1464097 , @sammccall wrote:

> Adding `-std=c++14` doesn't work in general as it has side-effects: `clang 
> -std=c++14 foo.c` is a warning, `clang -std=c++14 -x c-header foo.h` is an 
> error. It would be nice if clang had a flag to specify the default c++ 
> language version without also forcing the file to be parsed as C++, but AFAIK 
> it does not.


Hmm. We have `-std-default`, but apparently it only works in C. :( Shouldn't be 
too hard to fix that.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60539/new/

https://reviews.llvm.org/D60539



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

[PATCH] D60539: Add -std=c++14 language standard option to tests that require C++14 default

[Sam McCall via Phabricator via cfe-commits]

sammccall requested changes to this revision.
sammccall added a comment.
This revision now requires changes to proceed.
Herald added a subscriber: jsji.

I don't think this is a suitable fix :-(

There are lots of places where we construct command-lines in tests, it may be 
true today that this is the only one that fails with `gnucxx11`, but there are 
other possible values for `CLANG_DEFAULT_STD_CXX` and also code changes over 
time. So if we want to be robust to this we need a general approach to this 
that can be used in `SymbolCollectorTest`, `TestTU`, and others.

Adding `-std=c++14` doesn't work in general as it has side-effects: `clang 
-std=c++14 foo.c` is a warning, `clang -std=c++14 -x c-header foo.h` is an 
error. It would be nice if clang had a flag to specify the default c++ language 
version without also forcing the file to be parsed as C++, but AFAIK it does 
not.

> In our case, we set the default to be gnucxx11. However, doing so will cause 
> the test cases in this patch to fail as they rely on the C++14 default.

Do you need to build clangd? We explicitly don't aim to support building 
everywhere clang can be built, maybe we should just disable in this case?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60539/new/

https://reviews.llvm.org/D60539



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

[PATCH] D60539: Add -std=c++14 language standard option to tests that require C++14 default

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

LGTM

Generally it's a good thing for our test suite to be robust against changes to 
Clang's default language mode.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60539/new/

https://reviews.llvm.org/D60539



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

[PATCH] D60539: Add -std=c++14 language standard option to tests that require C++14 default

[Amy Kwan via Phabricator via cfe-commits]

amyk created this revision.
amyk added reviewers: ilya-biryukov, sammccall, ioeric, hokein, akyrtzi, yvvan.
amyk added projects: clang, LLVM.
Herald added subscribers: kadircet, arphaman, dexonsmith, jkorous.

On one of the platforms that we build on, we build with the CMake macro, 
`CLANG_DEFAULT_STD_CXX` to set the default language level when building Clang 
and LLVM.

In our case, we set the default to be `gnucxx11`. However, doing so will cause 
the test cases in this patch to fail as they rely on the C++14 default.

This patch explicitly adds the `-std=c++14` to the affected test cases so they 
will work when the default language level is set.

I have added individuals who have worked with these test cases in the past as 
reviewers. I would greatly appreciate it if any of you can inform me on whether 
or not this change is acceptable.


https://reviews.llvm.org/D60539

Files:
  clang-tools-extra/unittests/clangd/SymbolCollectorTests.cpp
  clang/test/Index/print-type-size.cpp


Index: clang/test/Index/print-type-size.cpp
===
--- clang/test/Index/print-type-size.cpp
+++ clang/test/Index/print-type-size.cpp
@@ -1,6 +1,6 @@
 // from SemaCXX/class-layout.cpp
-// RUN: c-index-test -test-print-type-size %s -target x86_64-pc-linux-gnu | 
FileCheck -check-prefix=CHECK64 %s
-// RUN: c-index-test -test-print-type-size %s -target i386-apple-darwin9 | 
FileCheck -check-prefix=CHECK32 %s
+// RUN: c-index-test -test-print-type-size %s -target x86_64-pc-linux-gnu 
-std=c++14 | FileCheck -check-prefix=CHECK64 %s
+// RUN: c-index-test -test-print-type-size %s -target i386-apple-darwin9 
-std=c++14 | FileCheck -check-prefix=CHECK32 %s
 
 namespace basic {
 
Index: clang-tools-extra/unittests/clangd/SymbolCollectorTests.cpp
===
--- clang-tools-extra/unittests/clangd/SymbolCollectorTests.cpp
+++ clang-tools-extra/unittests/clangd/SymbolCollectorTests.cpp
@@ -109,6 +109,7 @@
 File.Filename = FileName;
 File.HeaderCode = HeaderCode;
 File.Code = Code;
+File.ExtraArgs.push_back("-std=c++14");
 AST = File.build();
   }
 


Index: clang/test/Index/print-type-size.cpp
===
--- clang/test/Index/print-type-size.cpp
+++ clang/test/Index/print-type-size.cpp
@@ -1,6 +1,6 @@
 // from SemaCXX/class-layout.cpp
-// RUN: c-index-test -test-print-type-size %s -target x86_64-pc-linux-gnu | FileCheck -check-prefix=CHECK64 %s
-// RUN: c-index-test -test-print-type-size %s -target i386-apple-darwin9 | FileCheck -check-prefix=CHECK32 %s
+// RUN: c-index-test -test-print-type-size %s -target x86_64-pc-linux-gnu -std=c++14 | FileCheck -check-prefix=CHECK64 %s
+// RUN: c-index-test -test-print-type-size %s -target i386-apple-darwin9 -std=c++14 | FileCheck -check-prefix=CHECK32 %s
 
 namespace basic {
 
Index: clang-tools-extra/unittests/clangd/SymbolCollectorTests.cpp
===
--- clang-tools-extra/unittests/clangd/SymbolCollectorTests.cpp
+++ clang-tools-extra/unittests/clangd/SymbolCollectorTests.cpp
@@ -109,6 +109,7 @@
 File.Filename = FileName;
 File.HeaderCode = HeaderCode;
 File.Code = Code;
+File.ExtraArgs.push_back("-std=c++14");
 AST = File.build();
   }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits