intel / intel/llvm

How should duplicate attributes be handled?

Open
#3,162 10 comments 0 reactions 0 assignees View on GitHub
confirmed
Dominant language
LLVM
Stars
1.5k
Forks
854
Avg merge
3d 17h
Merged PRs (30d)
137

Description

We're inconsistent with how we report duplicate attributes and I'm wondering what direction we'd like to resolve the inconsistencies.

Some attributes check to see if the same attribute was already applied to the declaration, but do not check whether the arguments to the attributes are the same. We then issue a diagnostic saying that the attribute is applied with different parameters (which may be wrong!). e.g., https://github.com/intel/llvm/blob/sycl/clang/lib/Sema/SemaDeclAttr.cpp#L3194

Some attributes check the attribute argument values to see if they're different between the two attributes and only diagnose if they're different. e.g., https://github.com/intel/llvm/blob/sycl/clang/lib/Sema/SemaDeclAttr.cpp#L3180

Still others use a helper method that we introduced called `checkForDuplicateAttribute()`, which reports any kind of duplication regardless of arguments. We use this both for attributes that have no arguments (https://github.com/intel/llvm/blob/sycl/clang/lib/Sema/SemaDeclAttr.cpp#L5597) and attributes that have arguments (https://github.com/intel/llvm/blob/sycl/clang/lib/Sema/SemaDeclAttr.cpp#L5621).

In community, the usual approach is to only diagnose a duplicate attribute if the arguments are different between the attributes, and to warn + retain (https://github.com/llvm/llvm-project/blob/master/clang/lib/Sema/SemaDeclAttr.cpp#L2980) or to warn + drop the new attribute (https://github.com/llvm/llvm-project/blob/master/clang/lib/Sema/SemaDeclAttr.cpp#L3026, https://github.com/llvm/llvm-project/blob/master/clang/lib/Sema/SemaDeclAttr.cpp#L3227).

Given that we seem to almost uniformly warn + retain (except for https://github.com/intel/llvm/blob/sycl/clang/lib/Sema/SemaDeclAttr.cpp#L3295), I think the correct fix here is to not warn about duplicate attributes that have no arguments or when the attribute arguments are identical (which means we'll silently accumulate the duplicates in the AST). For attributes whose arguments differ, it may make sense to continue to diagnose the confusion, but should we drop one attribute or the other (or both)?

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.