[clang-tidy] Enable modernize-return-braced-init-list's tests in C++11

[localspook]

The check supports C++11:

llvm-project/clang-tools-extra/clang-tidy/modernize/ReturnBracedInitListCheck.h

Lines 25 to 27 in d5aa5e3

	bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
	return LangOpts.CPlusPlus11;
	}

but isn't tested in it.

[llvmbot]

@llvm/pr-subscribers-clang-tidy

Author: Victor Chernyakin (localspook)

Changes

The check supports C++11:

llvm-project/clang-tools-extra/clang-tidy/modernize/ReturnBracedInitListCheck.h

Lines 25 to 27 in d5aa5e3

	bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
	return LangOpts.CPlusPlus11;
	}

but isn't tested in it.

---

Full diff: https://github.com/llvm/llvm-project/pull/158196.diff

1 Files Affected:

• (modified) clang-tools-extra/test/clang-tidy/checkers/modernize/return-braced-init-list.cpp (+3-1)

diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/return-braced-init-list.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/return-braced-init-list.cpp
index 02e95e15499dc..1450a6d1e60ce 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/modernize/return-braced-init-list.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/return-braced-init-list.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy -std=c++14-or-later %s modernize-return-braced-init-list %t
+// RUN: %check_clang_tidy %s modernize-return-braced-init-list %t
 
 namespace std {
 typedef decltype(sizeof(int)) size_t;
@@ -80,10 +80,12 @@ Foo f2() {
   return {b2};
 }
 
+#if __cplusplus >= 201402L
 auto f3() {
   Bar b3;
   return Foo(b3);
 }
+#endif
 
 #define A(b) Foo(b)
 

[llvmbot]

@llvm/pr-subscribers-clang-tools-extra

Author: Victor Chernyakin (localspook)

Changes

The check supports C++11:

llvm-project/clang-tools-extra/clang-tidy/modernize/ReturnBracedInitListCheck.h

Lines 25 to 27 in d5aa5e3

	bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
	return LangOpts.CPlusPlus11;
	}

but isn't tested in it.

---

Full diff: https://github.com/llvm/llvm-project/pull/158196.diff

1 Files Affected:

• (modified) clang-tools-extra/test/clang-tidy/checkers/modernize/return-braced-init-list.cpp (+3-1)

diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/return-braced-init-list.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/return-braced-init-list.cpp
index 02e95e15499dc..1450a6d1e60ce 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/modernize/return-braced-init-list.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/return-braced-init-list.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy -std=c++14-or-later %s modernize-return-braced-init-list %t
+// RUN: %check_clang_tidy %s modernize-return-braced-init-list %t
 
 namespace std {
 typedef decltype(sizeof(int)) size_t;
@@ -80,10 +80,12 @@ Foo f2() {
   return {b2};
 }
 
+#if __cplusplus >= 201402L
 auto f3() {
   Bar b3;
   return Foo(b3);
 }
+#endif
 
 #define A(b) Foo(b)
 

[vbvictor]

LGTM

[carlosgalvezp]

The change will make it test only in C++17, won't it? Don't we want c++11-or-later instead?

[vbvictor]

The change will make it test only in C++17, won't it? Don't we want c++11-or-later instead?

It will be c++11-or-later by default:

llvm-project/clang-tools-extra/test/clang-tidy/check_clang_tidy.py

Lines 392 to 394 in 111de45

	if args.std is None:
	_, extension = os.path.splitext(args.assume_filename or args.input_file_name)
	args.std = ["c99-or-later" if extension in [".c", ".m"] else "c++11-or-later"]

[carlosgalvezp]

It will be c++11-or-later by default:

That's sounds very fragile, if the standard is important, the test should be explicit about it.

Otherwise anyone can bump that standard in run-clang-tidy.py in the future to something else and suddenly this test no longer tests C++11. CI won't catch that.

[vbvictor]

It will be c++11-or-later by default:

That's sounds very fragile, if the standard is important, the test should be explicit about it.

Otherwise anyone can bump that standard in run-clang-tidy.py in the future to something else and suddenly this test no longer tests C++11. CI won't catch that.

Maybe then we could fail test if std flag isn't passed to check_clang_tidy.py?
This will encourage all new developers to set c++ standard explicitly all the time.

[vbvictor]

Maybe then we could fail test if std flag isn't passed to check_clang_tidy.py? This will encourage all new developers to set c++ standard explicitly all the time.

This could be a problem for downstream users, but not a very significant one. To preserve current behavior, we could make a flag enable-default-standard or something similar.

[localspook]

Some statistics: if we decide to force specifying -std, we would need to fix ~620 test files (number found by looking for
*.{c,cpp,m,mm} files under clang-tools-extra/test/clang-tidy that don't contain the string -std). Doable if a bit tedious.

[carlosgalvezp]

Nah I don't think we should make it mandatory. There are checks that don't require any specific standard, there it would make sense to keep whatever the default is.

I would say it makes sense to specify it in the test when the check also specifies it as requirement.

[vbvictor]

There are checks that don't require any specific standard, there it would make sense to keep whatever the default is.

This could also backfire for users that use "old" c++11. Imagine if the default standard is increased in check_clang_tidy.py, then the people who develop checks (and write tests) will unintentionally use c++14 as the default mode. Matchers written with c++14 in mind, may not work in c++11 (I encountered such behavior while developing checks). So the clang-tidy users with std=c++11 will see that the check doesn't work.

[carlosgalvezp]

This could also backfire for users that use "old" c++11. Imagine if the default standard is increased in check_clang_tidy.py, then the people who develop checks (and write tests) will unintentionally use c++14 as the default mode. Matchers written with c++14 in mind, may not work in c++11 (I encountered such behavior while developing checks). So the clang-tidy users with std=c++11 will see that the check doesn't work.

Yes. I think we should think carefully about what the default standard means and should be, and when it should be bumped. If a check promises to work with "any" C++ standard (as is often the case), then it should in theory test all the standards it promises. Testing only C++11 or later risks the check not working for C++98 and C++03 for example.

In other words I think the default standard in run_clang_tidy.py should be the oldest version we want to support in testing, and that CI can afford. Hard to know when it should be bumped, but we should have some confidence that the majority of users no longer uses an old standard.

[vbvictor]

In other words I think the default standard in run_clang_tidy.py should be the oldest version we want to support in testing

Since we bumped version of many checks to c++11 as intended, maybe it's time to downgrade check_clang_tidy default version to c++98 and see what breaks.:)

[localspook]

Added -std=c++11-or-later. Although if we want this to be a general policy that all checks which require LangOpts.CPlusPlus11 should specify -std explicitly, we'll need to fix a number of tests besides this one