My commit adds "__builtin_FILE_NAME()" to clang, which insert only the filename because the full path is not always needed.
Details
Diff Detail
Event Timeline
clang/lib/AST/Expr.cpp | ||
---|---|---|
2292 | It looks like a copy of the code from ExpandBuiltinMacro since we are already calling processPathForFileMacro from that same file why not factor out this code? If we can avoid code duplication we should to prevent possible future refactors only updating one copy of the code. |
It'd be nice to have compiler feature parity. I created a GCC feature request for __builtin_FILE_NAME: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108978
I tried rebase revision, but edit other revision (https://reviews.llvm.org/D145040).
We need to rebase the revision, because the tests are falling, because there is no this commit: https://reviews.llvm.org/rG24d144571dbffc6993d13fb7ca781248eed024de
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?
clang/unittests/AST/ASTImporterTest.cpp | ||
---|---|---|
1553 | You should back out the spurious whitespace changes (I'd recommend setting your editor to not discard trailing whitespace on save as that's a common culprit for this). |
This is my first open-source commit.
I don't know where or what to write.
Maybe you will do it or tell me how to do it?
clang/unittests/AST/ASTImporterTest.cpp | ||
---|---|---|
1553 | Ok. |
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.
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.
These things are bag of bytes - we likely can't promise _anything_ because there is no requirements of filenames to be encodable in _any_ encoding on some systems. In practice we can assume it's UTF-8 everywhere and convert to UTF-8 on windows because that's the only string literal encoding clang supports for now. If we did support other literal encodings, we would need to answer that question. IE, we could try to convert, but the conversion might not work.
clang/include/clang/Lex/Preprocessor.h | ||
---|---|---|
2859 | processPathToFileName would be better for consistency | |
clang/lib/Lex/PPMacroExpansion.cpp | ||
1993–1994 | we probably want to pass a SmallVectorImpl<char> here, instead of a SmallString, to avoid making that function a template | |
2000–2001 |
clang/docs/ReleaseNotes.rst | ||
---|---|---|
113–114 ↗ | (On Diff #504648) | |
clang/include/clang/AST/Expr.h | ||
4689–4690 | ||
clang/lib/AST/Expr.cpp | ||
2283–2284 | ||
clang/lib/Lex/PPMacroExpansion.cpp | ||
2000–2002 | Minor coding style nit. | |
clang/lib/Parse/ParseExpr.cpp | ||
805–817 | It looks like the formatting here got wrecked, you should back these changes out. |
LGTM aside from a small formatting issue. Do you need someone to land this on your behalf? If so, what name and email address would you like used for patch attribution?
clang/lib/Parse/ParseExpr.cpp | ||
---|---|---|
804–805 | Formatting here also got messed up a bit. |
Yes, I need help with landing.
Name: Ilya Karapsin.
Email: ilya84050@gmail.com.
clang/lib/Parse/ParseExpr.cpp | ||
---|---|---|
804–805 | If these two lines are restored, "git-clang-format" will break the comments. |
clang/lib/Parse/ParseExpr.cpp | ||
---|---|---|
804–805 | I'll fix it on commit. Do you still get that behavior if you use git-clang-format to format only the *patch* though? https://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting |