search

llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
29.12k stars 12.01k forks source link

[Clang/Format] Don't use Style.isCpp() to check for attribute specifier availability #41990

Open bhamiltoncx opened 5 years ago

bhamiltoncx commented 5 years ago
Bugzilla Link 42645
Version trunk
OS All

Extended Description

As pointed out by aaron.ballman in review of D64632:

https://reviews.llvm.org/D64632#1582859

Clang has a feature flag to enable support for double-square bracket attributes in more than just C++ mode, and this is enabled by default in C2x mode. This check for isCpp() makes me suspect we may be doing the wrong thing here.

TokenAnnotator.cpp should be fixed to use this feature flag.

mydeveloperday commented 2 years ago

Do we mean the option Style.Standard=LS_Cpp20?

would we want clang-format to behave differently if this wasn't set?

I personally tend not to use that Standard and hope that we can make it work with whatever version incase users source trees are a mixture of multiple standards.

I don't think I'd want it to stop doing the right thing just because .clang-format said it was C++17 because to me that means C++17 or above.

@mkurdej any thoughts on this?

mydeveloperday commented 2 years ago

marking at least as an enhancement as I don't really see a bug here.

mkurdej commented 2 years ago

First and foremost, is there a real non-hypothetical bug?

Also, looking at this:

/// Should be used for C, C++.
    LK_Cpp,
/// Should be used for Objective-C, Objective-C++.
    LK_ObjC,
...
bool isCpp() const { return Language == LK_Cpp || Language == LK_ObjC; }

we see that isCpp is true for C, C++, ObjC and ObjC++. So this handles C2x or other C mode (we treat C as C++).

Additionally, I don't see how we'd like to pass any additional flags to clang-format apart from introducing Language: C/Standard: C2x.

To be honest, I'm inclined to close this issue.