[clang] [clang] Fix implicit integer conversion for opaque enums declared in class templates (PR #121039)

[via cfe-commits]

=?utf-8?q?André?= Brand <andre.brand at mailbox.org>
Message-ID:
In-Reply-To: <llvm.org/llvm/llvm-project/pull/121039 at github.com>


thebrandre wrote:

@cor3ntin Good news! I figured it out on my own! And I now have a fix that I am confident in. 🙂
I analyzed the steps of the parser in **a lot**  of similar cases, for which I also added quite a few regression tests.

The attributes have already been handled correctly previously. But to make sure of it, I also added tests.

Putting the few lines for the fix there, seems fairly consistent to me. It repeats the logic in [`Sema::ActOnTag`](https://github.com/llvm/llvm-project/blob/llvmorg-19.1.6/clang/lib/Sema/SemaDecl.cpp#L17152-L17154) and [`Sema::ActOnEnumBody`](https://github.com/llvm/llvm-project/blob/llvmorg-19.1.6/clang/lib/Sema/SemaDecl.cpp#L19917-L19922) (exact lines of code are linked). We would now repeat them a third time but this is unavoidable because `Sema::ActOnEnumBody` is called again during instantiation but `Sema::ActOnTag` is not.
Refactoring this is would be hard because it's not really specified what exactly the individual functions are supposed to do. And the repetition doesn't seem too bad to me.

As you suggested, I stepped through the code to check if anything else from `Sema::ActOnTag` might be missing. But this doesn't seem to be the case.
Some things seemed odd but non-relevant. For example for `enum E : int {};`  `EnumDecl::NumPositiveBits` is set to 1, but it stays 0 for the opaque declaration `enum E : int;`. I also noticed that I can trigger a case where [a diagnostic gets reported twice](https://godbolt.org/z/473Eezhjn)). This, however, seems rather unrelated to me. I would rather open up a separate pull request for that.

Thanks again for your feedback @cor3ntin - it helped me to get a much cleaner solution and a better understanding of the issue even though my initial analysis might have been a little confusing ...
Well, I hope it's now ready for merging. 🙂

I rebased my branch on the current main and also "rebased away" my prior fix to make the diff more concise.


https://github.com/llvm/llvm-project/pull/121039