[PATCH] D146986: Downgrade reserved module identifier error into a warning

Mon Mar 27 12:28:57 PDT 2023

dblaikie added a comment.

In D146986#4225192 <https://reviews.llvm.org/D146986#4225192>, @aaron.ballman wrote:

> In D146986#4225121 <https://reviews.llvm.org/D146986#4225121>, @dblaikie wrote:
>
>> From the discussion on the issue:
>>
>>> Do we want this loosening of the restriction to apply to *only* `std` and the same followed by a number, or to any reserved identifier used in a module? e.g.,
>>>
>>>   module std; // error today, but will become a warning
>>>   module _Test; // error today, but do we want this to become a warning as well?
>>>
>>> my thinking is we probably want all of these to be warnings because it'd be hard to explain why `std` is reserved but with a warning while `_Test` is reserved but with an error.
>>
>> Yeah, I'd treat them equally - while we could subset the reserved names and allow implementations to only use a subset (while leaving the rest as an error for both implementations and consumers alike) that doesn't feel in keeping with the purpose of these names - to be usable by /someone/ and so necessary to allow them to be used.
>>
>> (hmm - there's some discussion in the description about the fact that this error was already suppressed in "system headers" - why was that suppression inadequate for system implementation modules? (& does that suppression for reserved names risk being over-broad, since every third party library installed on a system is generally considered a "system header", even if they aren't part of the implementation?))
>
> We currently use line markers to "enter" a system header and that's quite fragile. I mentioned we could use `#pragma clang system_header`, but @ChuanqiXu  didn't think that was appropriate because these are not headers, they're modules, and we should have some separation between "system headers" and "system modules". (https://github.com/llvm/llvm-project/issues/61446#issuecomment-1473029776) As for being over-broad, it might be, but this is the approach we usually take (anything that's a "system header" is considered special and gets less diagnostics because the user isn't typically able to change the contents of the header file anyway).

Presumably adding an alias for `#pragma clang system_header` called `system_module` wouldn't be too hard? (though the pragma is also being removed from libc++ soon, I think, in favor of `-isystem` usage, so maybe that's a sign the pragma's not a great way to do)

Presumably the line marker issue would be less significant for a module? Since there's no complex line marking, inclusion, etc, going on - it's just where the actual .cppm file is located/how it's found? (though yeah, that might get weird for building .pcms - since you're going to name the source file directly on the command line, it's not going to be found via any isystem lookup, etc... )

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146986