It seems to me like the warning is valid, even though we use precompiled headers.

Is this behavior specified somewhere? Or are we simply adding an extension to Clang?

Looks ok to me.

Bump!

I do not want to promote all promotable allocas, I only would like to target cleanup allocas (flags and dest.slot),

Why? It sounds like you're trying to optimize some aspect of the -O0 lowering.

I'd love to see this approved. It's a bad bug.

This appears to be only measuring the time spent instantiating class templates. What about other template entities, like functions?

I just ran into this problem while trying to get range-v3 working on clang-3.6. This is_function implementation probably instantiates fewer templates than the version currently shipping with libc++. Disclaimer: I haven't tested it exhaustively yet.

Nope looks good.

My question was more about whether the code is correct for absolute paths on Windows, not about whether this particular test would pass or fail. Have you tested an incorrectly-cased absolute path on a Windows machine?

s/constexpr/const/

Win2008 buildbot still unhappy. Sorry about this guys. I suppose it's the constexpr. We don't have a win2008 machine locally to test on. Is there a way for us to run this through your gauntlet without actually doing a commit?

s/std::size_t/size_t/ to keep the Win2008 buildbot happy.

FormatTests unittest passes for me with just my patch. Not sure which diff is causing that failure but it seems not to be mine.

Looks like I need s/std::size_t/size_t/ to keep the win2008 buildbot happy. Also, Taewook and I are looking into the clang-format test failure, but my suspicion is that that one isn't ours.

@rsmith wrote:

Thanks for the updates, LGTM (@bruno, did you get the performance numbers you wanted?)

Replace StringSet with StringSwitch, ASCII range ends at 0x7f not 0xff, miscellaneous formatting tweaks.

Before this goes in again, I want to double check that this doesn't affect compile time on darwin + frameworks.

Reopening this and the updated D19843.

Reopening. I would like some eyes on the updated patch before we merge. @rsmith Would you prefer this (and D19842) in new diffs?

Rework the patch to only warn by default for include files not found in system include directories, unless they are known "standard" headers that should be portable (C/C++/Posix/Boost). Add an additional warning -Wnonportable-system-include-path for turning the warning on for user code that #include's other system headers like <Windows.h>. (As always, warning within system headers are suppressed.)

make hasProcSelfFD() static

Add unittest for openFileForRead; assorted stylistic tweaks.

Remove stale comment, tweak for diagnostic wording.

Add getPathFromOpenFD and unittest, as requested.

Awesome, thanks.

Add fixit tests, fix path separator fixit issue on Windows.

Please add some tests for the FixItHint replacement text. See test/FixIt for examples of how to do this.

What I'm suggesting here is that you abstract everything that guarantees returning the realpath (as in absolute, etc) into a separate function, lets call it canonical_path, even if that means that the only reliable option is calling the realpath function itself. Since on windows it's a different function, I see much value in having that abstracted out into a OS independent function.

Thanks for the feedback, @curdeius. What happens now? Do I just wait until somebody accepts the llvm diff and this one? How do I increase the likelihood that that happens?

Factor out TrySimplifyPath from Preprocessor::HandleIncludeDirective. Other review feedback.

Any more comments on this one? @majnemer? @bruno?

clang has been using realpath in the FileManager for a long time and it seems to be working fine, I'm not following why it isn't 100% correct. In the VFS, as far as the needs go, it has been used to detect symlink in the components and work as expected.

The only way I know to make realpath 100% correct 100% of the time is to readdir starting from the drive root, enumerating all the subdirectories to find one that matches the next path component, and recursing all the way up. I think I'm safe in saying that making roughly a bajillion API calls that hit the disk over and over is going to be a lot slower than making just one and hoping for the best. :-)

Can you split the functionality that gets the RealPath into a new function (something like llvm::sys::fs::canonical() or llvm::sys::fs::realpath()) and make openFileForRead calls it?
[...]
This is going to be useful for a bunch of other client in clang that are now obtaining realpath in a adhoc/slow ways.

Review feedback

See D19843 for the corresponding cfe changes that implements the new diagnostic.

You could wrap the templated default ctor in a feature-test macro. This is done many other places in the library.

Pretty sure defaulted template parameters are used elsewhere already, are they not? Would adding an argument with a default change is_trivial for pairs?

Yes and no. It also means getting the answer right for std::is_default_constructible<pair<int&,int&>>::value. Right now, it says "true", when it's more accurate to say "false".

The full fix is to go through *all* the constructors and assignment operators of pair and tuple and apply the same change. tuple already does this in many places, but it's inconsistent, and pair doesn't do it at all.