Add -std=c++14 language standard option to tests that require C++14 default

[amyk]

Summary

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.

[rsmith]

LGTM

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

[sammccall]

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?

[rsmith]

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.

[nemanjai]

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?

[sammccall]

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

clang-tools-extra/unittests/clangd/SymbolCollectorTests.cpp

 
 class ShouldCollectSymbolTest : public ::testing::Test {
 public:
   void build(llvm::StringRef HeaderCode, llvm::StringRef Code = "") {
     File.HeaderFilename = HeaderName;
     File.Filename = FileName;
     File.HeaderCode = HeaderCode;
     File.Code = Code;
+    File.ExtraArgs.push_back("-std=c++14");
     AST = File.build();
   }
 
   // build() must have been called.
   bool shouldCollect(llvm::StringRef Name, bool Qualified = true) {
     assert(AST.hasValue());
     const NamedDecl& ND = Qualified ? findDecl(*AST, Name) 
                                     : findUnqualifiedDecl(*AST, Name);

clang/test/Index/print-type-size.cpp

 // 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 {
 
 // CHECK64: VarDecl=v:[[@LINE+2]]:6 (Definition) (invalid) [type=void] [typekind=Void]
 // CHECK32: VarDecl=v:[[@LINE+1]]:6 (Definition) (invalid) [type=void] [typekind=Void]
 void v;
 
 // CHECK64: VarDecl=v1:[[@LINE+2]]:7 (Definition) [type=void *] [typekind=Pointer] [sizeof=8] [alignof=8]