[PATCH] D144878: __builtin_FILE_NAME()

[Aaron Ballman via Phabricator via llvm-commits]

aaron.ballman added subscribers: cor3ntin, tahonermann.
aaron.ballman added a comment.

In D144878#4183893 <https://reviews.llvm.org/D144878#4183893>, @karapsinie wrote:

> In D144878#4172639 <https://reviews.llvm.org/D144878#4172639>, @aaron.ballman wrote:
>
>> In D144878#4171234 <https://reviews.llvm.org/D144878#4171234>, @karapsinie wrote:
>>
>>> PTAL.
>>
>> Have you seen the comments on the GCC issue that @MaskRay filed? Is that something we should do as well? (It doesn't have to be part of this patch, but it'd be good to ensure we're collaborating with GCC so we get the same builtin functionality in this space.)
>>
>> There should be a release note and documentation for the new functionality. One thing to consider for the docs is explaining what's going on with text encodings (and perhaps that text applies to this entire class of file-related builtins). e.g., if the system code page is Shift-JIS does this builtin return the text in Shift-JIS or UTF-8 or something else?
>
> This is my first open-source commit.

Oh, awesome! Welcome!

> I don't know where or what to write.
> Maybe you will do it or tell me how to do it?

Happily!

Our release notes live in `clang/docs/ReleaseNotes.rst` and I'd probably add it under this heading specifically: https://github.com/llvm/llvm-project/blob/main/clang/docs/ReleaseNotes.rst#non-comprehensive-list-of-changes-in-this-release
Our documentation for builtins live in `clang/docs/LanguageExtensions.rst` and I'd probably add documentation under this heading specifically: https://github.com/llvm/llvm-project/blob/main/clang/docs/LanguageExtensions.rst#source-location-builtins

Both our documentation and our release notes are written in Sphinx (RST) which is a markdown format, more details of which can be found at: https://www.sphinx-doc.org/en/master/usage/restructuredtext/basics.html

In terms of the encoding question I was asking, that's information we'll have to figure out (CC @tahonermann and @cor3ntin for text encoding question). My guess (which needs verification) is that we convert the file name from the system encoding to UTF-8 internally, and this builtin will likely return UTF-8 as a result. If that's correct, I think that behavior is reasonable, but I've CCed some experts who can tell me all the things I forgot to consider.

As for speaking with GCC devs, I think the issue that @MaskRay filed (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108978) got the discussion started. They asked if we should add `__builtin_BASE_FILE` at the same time -- is that something you think we should add and would be willing to work on as well? If so, then you should probably add a comment to that bug saying that's something we'd be fine adding so the GCC folks know the scope of the request. And if that's not something you think should be added, you should probably add a comment to that bug saying so and why. FWIW, you can see the implementation for `__BASE_FILE__` and `__FILE_NAME__` here: https://github.com/llvm/llvm-project/blob/main/clang/lib/Lex/PPMacroExpansion.cpp#L1537.


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

https://reviews.llvm.org/D144878