[PATCH] D110668: [clang-cl] Accept `#pragma warning(disable : N)` for some N

Aaron Ballman via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Oct 4 10:59:07 PDT 2021


aaron.ballman added a comment.

In D110668#3038858 <https://reviews.llvm.org/D110668#3038858>, @xbolva00 wrote:

> In D110668#3036361 <https://reviews.llvm.org/D110668#3036361>, @thakis wrote:
>
>> In D110668#3034576 <https://reviews.llvm.org/D110668#3034576>, @xbolva00 wrote:
>>
>>> Please next time give a bit more time to potential reviewers / other folks outside your org. The whole lifecycle of this patch (posted - landed) took < 24h.
>>
>> Is there anything wrong with the patch?
>>
>> I agree that it's good to let larger changes sit for a bit, but this seems like a fairly small and inconsequential change to me. Many patches land with a review time < 24h.
>>
>> In any case, happy to address post-commit review comments too of course.
>
> I think I would prefer to implement such "mapping" in DiagnosticGroups.td instead of current solution. cc @aaron.ballman as well, as he is an exprienced reviewer here.
>
> What I mean,  for example:
>
>   def UnusedParameter : DiagGroup<"unused-parameter", 4100>;

FWIW, I agree that this is a better approach -- I'm uncomfortable having a separate list of diagnostic IDs that matter. To me, this information is part of the diagnostic itself. Having it listed alongside the diagnostic text also means that when we repurpose a diagnostic to have a slightly different meaning (which does happen from time to time), there's something more visible in the reviewer's face about whether the change also means the diagnostic shouldn't map to a CL diagnostic (or should map to a different one). Personally, I would prefer to see it surfaced like this:

`def UnusedParameter : DiagGroup<"unused-parameter", [CL<4100>]>;`

so that it accepts a list of alternative IDs for the diagnostic. We currently only care about the stable numbering from cl.exe, but there's no reason we wouldn't want to use this for other kinds of diagnostic grouping aliases. For example, someday we may want to use these same facilities to map between Clang diagnostic groups and GCC diagnostic groups where we surfaced different names for the group by accident.

In D110668#3040408 <https://reviews.llvm.org/D110668#3040408>, @thakis wrote:

> That's an interesting idea.
>
> Given how seldom this is used, I weakly prefer having actual code for this though: It makes it easy to see all of those mapped flags, and it keeps rarely used things out of tablegen, which is imho a good thing.

If we want to make it easy to see all the mapped flags, we can add a -cc1 option to dump them out in a more consumable manner, so I think we have solutions we can use there. I don't see this as being a rarely used thing though -- it's the stable identifier for the diagnostic, so it strongly relates to the definition of the diagnostic itself. This comes up in user-facing ways like through `#pragma warning`, so it seems worth it to have it in tablegen, at least to me.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110668/new/

https://reviews.llvm.org/D110668



More information about the cfe-commits mailing list