[PATCH] Warn when an anonymous warning group is used more than once

[+cfe-dev, -cfe-commits for the new “coding” convention]

Hi, everyone. One thing that’s bothered me about our diagnostic TableGen files is the anonymous use of warning groups:

def warn_foo : Warning<“foo is bad”>, InGroup<DiagGroup<“foo”>>;
def warn_foo_cxx : Warning<“foo is even worse in C++”>, InGroup<DiagGroup<“foo”>>;

If we ever want to put the warning groups in a hierarchy, or change the name, there’s a big chance we’ll miss one.

So, I wrote a new warning for clang-tblgen, which even has a fixit if the group already has a name:

warning: group ‘foo’ is referred to anonymously
def warn_foo : Warning<“foo is bad”>, InGroup<DiagGroup<“foo”>>;

InGroup<Foo>

And then fixed all the warnings, with the intent that we make this a new style requirement going forwards: groups that cover more than one diagnostic must be named.

Sean has suggested making this an error instead of a warning, since we go for a warning-free build anyway. I’m not sure why I thought a warning was preferable; we’d obviously make anyone fix their code before committing if they triggered the warning. The only thing I can think of is that it might be better for quick testing when transitioning from one diagnostic to another, or when trying out a new diagnostic under an existing flag.

Opinions? I’m fine either way.

Jordan

Making this an error sounds fine to me.

[+cfe-dev, -cfe-commits for the new “coding” convention]

Hi, everyone. One thing that’s bothered me about our diagnostic TableGen files is the anonymous use of warning groups:

def warn_foo : Warning<“foo is bad”>, InGroup<DiagGroup<“foo”>>;
def warn_foo_cxx : Warning<“foo is even worse in C++”>, InGroup<DiagGroup<“foo”>>;

If we ever want to put the warning groups in a hierarchy, or change the name, there’s a big chance we’ll miss one.

So, I wrote a new warning for clang-tblgen, which even has a fixit if the group already has a name:

warning: group ‘foo’ is referred to anonymously
def warn_foo : Warning<“foo is bad”>, InGroup<DiagGroup<“foo”>>;

InGroup<Foo>

And then fixed all the warnings, with the intent that we make this a new style requirement going forwards: groups that cover more than one diagnostic must be named.

Sean has suggested making this an error instead of a warning, since we go for a warning-free build anyway. I’m not sure why I thought a warning was preferable; we’d obviously make anyone fix their code before committing if they triggered the warning. The only thing I can think of is that it might be better for quick testing when transitioning from one diagnostic to another, or when trying out a new diagnostic under an existing flag.

Opinions? I’m fine either way.

I’d prefer an error.

+1

Committed r172087-8. Thanks for the comments!