Page MenuHomePhabricator

Add a __FILE_NAME__ macro.
ClosedPublic

Authored by kristina on May 9 2019, 1:19 PM.

Diff Detail

Repository
rC Clang

Event Timeline

kristina created this revision.May 9 2019, 1:19 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 9 2019, 1:19 PM
kristina updated this revision to Diff 198894.May 9 2019, 1:28 PM

Fix style, remove unnecessary braces, add missing newline.

kristina edited the summary of this revision. (Show Details)May 9 2019, 1:51 PM
shawnl added a subscriber: shawnl.May 9 2019, 1:54 PM
shawnl added inline comments.
lib/Lex/PPMacroExpansion.cpp
1510

What is the path name uses both \ and / to separate paths? I think this needs to be:

if (LangOpts.MicrosoftExt) {
  size_t BackSlash = PLFileName.take_back(PLFileName.size() - LastSep).find_last_of('\\');
  if (BackSlash != StringRef::npos)
    LastSep = BackSlash;
}
shawnl requested changes to this revision.May 9 2019, 2:03 PM

There is no documentation specific to the #include directive. https://docs.microsoft.com/en-us/cpp/preprocessor/hash-include-directive-c-cpp?view=vs-2019

However, fileio treats / and \ the same, *sometimes*. https://docs.microsoft.com/en-us/windows/desktop/FileIO/naming-a-file

This revision now requires changes to proceed.May 9 2019, 2:03 PM
kristina updated this revision to Diff 199104.May 10 2019, 3:53 PM
kristina edited the summary of this revision. (Show Details)

Actually I got it wrong, the path is normalized to use regular slashes at that point, so there is no point in handling backslashes in paths at all even with Microsoft extensions.

After running it with a hack to dump the paths to stderr they seem to be the same regardless.

[FILENAME] /llvm-tainted/tools/clang/test/Preprocessor/Inputs/include-subdir/subdir1/hdr1.h
[FILENAME] /llvm-tainted/tools/clang/test/Preprocessor/Inputs/include-subdir/subdir1/hdr2.h

I've also extended the tests to cover scenarios with mixed slashes in MS compatibility mode and removed the unnecessary check.

NSProgrammer accepted this revision.May 10 2019, 4:11 PM
shawnl accepted this revision.May 10 2019, 4:25 PM
This revision is now accepted and ready to land.May 10 2019, 4:25 PM

Need @rsmith to bless this as it's introducing a nonstandard extension, however small it may be. The original diff did have a consensus on it, so I didn't really put up a formal RFC on cfe-dev.

rsmith added a subscriber: alexr.May 15 2019, 4:59 PM

Generally, I think this is a good, useful feature, but there's one point from http://clang.llvm.org/get_involved.html 's checklist for accepting language extensions on which its case is weak, as reflected by this feedback from @alexr on D17741:

I don't think we want to add a fundamental new preprocessor feature like __FILE_BASENAME__ without at least getting some early buy-in from the WG14 and WG21 committees.

Specifically: have you suggested this feature on the WG14 or WG21 mailing lists? I'm not especially opposed to carrying this feature as a Clang extension, but from http://clang.llvm.org/get_involved.html, "Clang should drive the standard, not diverge from it", and we should at least try to push for standardizing this if we think it's worthwhile as an alternative to __FILE__.

Failing that, have you asked the GCC developers whether they'd be amenable to providing this functionality?

Landing this as discussed on IRC, will try to push it forward with WG14.

I think having something like this as part of the standard would benefit a lot, since currently the "unofficial" way of doing this for either GCC or Clang involves several nonstandard builtins and __FILE__ which causes difficulties in reproducible builds as code emitted in read only data sections may still differ depending on optimization levels and other factors. This macro on the other hand provides a guarantee that only the last path component is rendered.

This revision was automatically updated to reflect the committed changes.
kristina reopened this revision.May 15 2019, 8:40 PM

Reverted in rL360842 as Windows bots were failing.

I suspect the MSVCCompat case may need to be handled differently depending on the host OS similar to PPDirectives.cpp:

  if (LangOpts.MSVCCompat) {
    NormalizedPath = Filename.str();
#ifndef _WIN32
    llvm::sys::path::native(NormalizedPath);
#endif
  }

Reopening this for now, will try to get a local Windows builder set up and work out what the problem is before relanding.

This revision is now accepted and ready to land.May 15 2019, 8:40 PM
Ka-Ka added a subscriber: Ka-Ka.May 15 2019, 11:23 PM

Reverted in rL360842 as Windows bots were failing.

I suspect the MSVCCompat case may need to be handled differently depending on the host OS similar to PPDirectives.cpp:

  if (LangOpts.MSVCCompat) {
    NormalizedPath = Filename.str();
#ifndef _WIN32
    llvm::sys::path::native(NormalizedPath);
#endif
  }

Reopening this for now, will try to get a local Windows builder set up and work out what the problem is before relanding.

Can't you just always use llvm::sys::path::filename(PLFileName)? It defaults to the native style.

kristina updated this revision to Diff 199882.May 16 2019, 12:26 PM
kristina edited the summary of this revision. (Show Details)

Revised to use llvm::sys::path::filename to avoid issues on Windows hosts.

This revision was automatically updated to reflect the committed changes.