llvm / llvm-project

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

No diagnostic is reported for illegal macro definitions such as "noreturn" and "carries_dependency" #92196

Open romanova-ekaterina opened 4 months ago

romanova-ekaterina commented 4 months ago

Consider the testcase like that:

#define override 1 
#define noreturn 2
#define final 3 
#define carries_dependency 4

int foo(int a)
{
  return (a == override + noreturn + final + carries_dependency);
}

When compiling it with clang compiler, I see that the diagnostic is issued for 2 out 4 macros (override and final), which is inconsistent.

I could see the argument why no error should be reported. I could see an argument why all four errors should be reported. But not reporting diagnostic only for 2 out of 4 macros that falls into the same category is suprising.

(note: the same behavior happens with c++14, c++17, c++20 standard)

clang -c -Wpedantic -Wall -std=c++11 test.cpp
test.cpp:2:9: warning: keyword is hidden by macro definition [-Wkeyword-macro]
#define override 1
        ^
test.cpp:4:9: warning: keyword is hidden by macro definition [-Wkeyword-macro]
#define final 3
        ^
2 warnings generated.
=======================

On one hand, https://eel.is/c++draft/macro.names#2 says: "A translation unit shall not #define or #undef names lexically identical to keywords, to the identifiers listed in Table 4, or to the attribute-tokens described in [dcl.attr], except that the names likely and unlikely may be defined as function-like macros ([cpp.replace]). "Table 4" is: "final import module override". Redefining attribute-tokens is also not allowed. https://eel.is/c++draft/dcl.attr; "carries_dependency" and "noreturn" are both in there (attribute tokens).

The use of the language "shall not" means that the program is ill-formed and requires a diagnostic, according to cppreference.

On the other hand, it might be considered as and undefined bahavior. There is a 10 years old thread on llvm.org explaining why error should not be reported. [However, it doesn't address my inconsistency question] https://lists.llvm.org/pipermail/cfe-dev/2013-June/030283.html

Summary: If Clang diagnoses a redefinition some attribute names such as "final" and "override", then failing to do so for "carries_dependency" and "noreturn" seems like a Clang bug.

llvmbot commented 4 months ago

@llvm/issue-subscribers-clang-frontend

Author: Katya Romanova (romanova-ekaterina)

Consider the testcase like that: #define override 1 #define noreturn 2 #define final 3 #define carries_dependency 4 int foo(int a) { return (a == override + noreturn + final + carries_dependency); } When compiling it with clang compiler, I see that the diagnostic is issued for 2 out 4 macros (override and final), which is inconsistent. I could see the argument why no error should be reported. I could see an argument why all four errors should be reported. But not reporting diagnostic only for 2 out of 4 macros that falls into the same category is suprising. (note: the same behavior happens with c++14, c++17, c++20 standard) clang -c -Wpedantic -Wall -std=c++11 test.cpp test.cpp:2:9: warning: keyword is hidden by macro definition [-Wkeyword-macro] #define override 1 ^ test.cpp:4:9: warning: keyword is hidden by macro definition [-Wkeyword-macro] #define final 3 ^ 2 warnings generated. ======================= On one hand, https://eel.is/c++draft/macro.names#2 says: "A translation unit shall not #define or #undef names lexically identical to keywords, to the identifiers listed in Table 4, or to the attribute-tokens described in [dcl.attr], except that the names likely and unlikely may be defined as function-like macros ([cpp.replace]). "Table 4" is: "final import module override". Redefining attribute-tokens is also not allowed. https://eel.is/c++draft/dcl.attr; "carries_dependency" and "noreturn" are both in there (attribute tokens). The use of the language "shall not" means that the program is ill-formed and requires a diagnostic, according to cppreference. On the other hand, it might be considered as and undefined bahavior. There is a 10 years old thread on llvm.org explaining why error should not be reported. [However, it doesn't address my inconsistency question] https://lists.llvm.org/pipermail/cfe-dev/2013-June/030283.html Summary: If Clang diagnoses a redefinition some attribute names such as "final" and "override", then failing to do so for "carries_dependency" and "noreturn" seems like a Clang bug.
shafik commented 1 month ago

It does not seem like other compilers complain about this: https://godbolt.org/z/EbrMjEG69

zygoloid commented 1 month ago

The use of the language "shall not" means that the program is ill-formed and requires a diagnostic, according to cppreference.

The context is "[a] translation unit that includes a standard library header", and this is in library wording where "shall" doesn't have quite the same implication as in core wording. Note also that clang doesn't issue an error here on any of these cases even in -pedantic-errors mode.

I don't think it's inconsistent to warn on context-sensitive keywords but not on attribute names, given that attribute names aren't any kind of keyword, and attributes are still usable via __decorated__ names for code that wants to be careful about such things.