Re: [PATCH] D19843: Use the name of the file on disk to issue a new diagnostic about non-portable #include and #import paths.

bruno added a comment. FTR, I finally test it out for compile time and could not notice any difference besides noise. Thanks Eric! http://reviews.llvm.org/D19843 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

Re: [PATCH] D19843: Use the name of the file on disk to issue a new diagnostic about non-portable #include and #import paths.

twoh added a comment. Patch committed in http://reviews.llvm.org/rL272592. http://reviews.llvm.org/D19843 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D19843: Use the name of the file on disk to issue a new diagnostic about non-portable #include and #import paths.

eric_niebler updated this revision to Diff 60615. eric_niebler added a comment. s/`constexpr`/`const`/ http://reviews.llvm.org/D19843 Files: include/clang/Basic/DiagnosticLexKinds.td include/clang/Basic/FileManager.h include/clang/Basic/VirtualFileSystem.h

Re: [PATCH] D19843: Use the name of the file on disk to issue a new diagnostic about non-portable #include and #import paths.

eric_niebler added a comment. 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? http://reviews.llvm.org/D19843

Re: [PATCH] D19843: Use the name of the file on disk to issue a new diagnostic about non-portable #include and #import paths.

twoh closed this revision. twoh added a comment. Committed by commit http://reviews.llvm.org/rL272584 http://reviews.llvm.org/D19843 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D19843: Use the name of the file on disk to issue a new diagnostic about non-portable #include and #import paths.

eric_niebler updated this revision to Diff 60584. eric_niebler added a comment. s/`std::size_t`/`size_t`/ to keep the Win2008 buildbot happy. http://reviews.llvm.org/D19843 Files: include/clang/Basic/DiagnosticLexKinds.td include/clang/Basic/FileManager.h

Re: [PATCH] D19843: Use the name of the file on disk to issue a new diagnostic about non-portable #include and #import paths.

twoh added a comment. I already reverted the path in http://reviews.llvm.org/rL272572, so re-commit should be the way to go. http://reviews.llvm.org/D19843 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

Re: [PATCH] D19843: Use the name of the file on disk to issue a new diagnostic about non-portable #include and #import paths.

eric_niebler added a comment. FormatTests unittest passes for me with just my patch. Not sure which diff is causing that failure but it seems not to be mine. Shall Taewook just commit the s/std::size_t/size_t/ change, or does something else need to be done (revert & resubmit)?

Re: [PATCH] D19843: Use the name of the file on disk to issue a new diagnostic about non-portable #include and #import paths.

eric_niebler reopened this revision. eric_niebler added a comment. This revision is now accepted and ready to land. 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

Re: [PATCH] D19843: Use the name of the file on disk to issue a new diagnostic about non-portable #include and #import paths.

twoh closed this revision. twoh added a comment. Re-commited in http://reviews.llvm.org/rL272562 http://reviews.llvm.org/D19843 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D19843: Use the name of the file on disk to issue a new diagnostic about non-portable #include and #import paths.

eric_niebler added a comment. @rsmith wrote: > Thanks for the updates, LGTM (@bruno, did you get the performance numbers you > wanted?) FWIW, I benchmarked (on Windows, where problems are likely to occur) with 3 implementations of `warnByDefaultOnWrongCase`: 1) `StringSwitch`, 2)

Re: [PATCH] D19843: Use the name of the file on disk to issue a new diagnostic about non-portable #include and #import paths.

rsmith added a comment. Thanks for the updates, LGTM (@bruno, did you get the performance numbers you wanted?) http://reviews.llvm.org/D19843 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

Re: [PATCH] D19843: Use the name of the file on disk to issue a new diagnostic about non-portable #include and #import paths.

eric_niebler updated this revision to Diff 60281. eric_niebler added a comment. Replace `StringSet` with `StringSwitch`, ASCII range ends at 0x7f not 0xff, miscellaneous formatting tweaks. http://reviews.llvm.org/D19843 Files: include/clang/Basic/DiagnosticLexKinds.td

Re: [PATCH] D19843: Use the name of the file on disk to issue a new diagnostic about non-portable #include and #import paths.

eric_niebler added inline comments. Comment at: lib/Lex/PPDirectives.cpp:33 @@ -28,2 +32,2 @@ #include "llvm/Support/Path.h" #include "llvm/Support/SaveAndRestore.h" rsmith wrote: > eric_niebler wrote: > > You mean, instead of the `StringSet` below? Looks like

Re: [PATCH] D19843: Use the name of the file on disk to issue a new diagnostic about non-portable #include and #import paths.

eric_niebler added a comment. > Before this goes in again, I want to double check that this doesn't affect > compile time on darwin + frameworks. @bruno, you're not likely to find a difference for darwin + frameworks since the frameworks headers like `Cocoa/Cocoa.h` don't exist on-disk with

Re: [PATCH] D19843: Use the name of the file on disk to issue a new diagnostic about non-portable #include and #import paths.

bruno added inline comments. Comment at: lib/Lex/PPDirectives.cpp:179 @@ +178,3 @@ +"exception", "iterator", "random", "strstream", "vector", +"forward_list", "limits", "ratio", "system_error", + Applying your patch reveled a bunch of ^M (carriage-return)

Re: [PATCH] D19843: Use the name of the file on disk to issue a new diagnostic about non-portable #include and #import paths.

rsmith added inline comments. Comment at: lib/Lex/PPDirectives.cpp:33 @@ -28,2 +32,2 @@ #include "llvm/Support/Path.h" #include "llvm/Support/SaveAndRestore.h" eric_niebler wrote: > You mean, instead of the `StringSet` below? Looks like `StringSwitch` just >

Re: [PATCH] D19843: Use the name of the file on disk to issue a new diagnostic about non-portable #include and #import paths.

eric_niebler added inline comments. Comment at: lib/Lex/PPDirectives.cpp:220 @@ +219,3 @@ +// In the ASCII range? +if (Ch < 0 || Ch > 0xff) + return false; // Can't be a standard header rsmith wrote: > Comment doesn't match code: the ASCII range ends

Re: [PATCH] D19843: Use the name of the file on disk to issue a new diagnostic about non-portable #include and #import paths.

eric_niebler added inline comments. Comment at: lib/Lex/PPDirectives.cpp:33 @@ -28,2 +32,2 @@ #include "llvm/Support/Path.h" #include "llvm/Support/SaveAndRestore.h" You mean, instead of the `StringSet` below? Looks like `StringSwitch` just turns into

Re: [PATCH] D19843: Use the name of the file on disk to issue a new diagnostic about non-portable #include and #import paths.

rsmith added a comment. Looks OK to me, pending Bruno's confirmation that performance is acceptable. Comment at: lib/Lex/PPDirectives.cpp:102 Can you use `llvm::StringSwitch` for this? I'd be interested to see how the performance compares.

Re: [PATCH] D19843: Use the name of the file on disk to issue a new diagnostic about non-portable #include and #import paths.

bruno added a comment. Before this goes in again, I want to double check that this doesn't affect compile time on darwin + frameworks. Will get back to you asap. Comment at: lib/Lex/PPDirectives.cpp:218 @@ +217,3 @@ + SmallString<32> LowerInclude{Include}; + for (char& Ch :

Re: [PATCH] D19843: Use the name of the file on disk to issue a new diagnostic about non-portable #include and #import paths.

eric_niebler reopened this revision. eric_niebler added a subscriber: bogner. eric_niebler added a comment. This revision is now accepted and ready to land. Reopening. I would like some eyes on the updated patch before we merge. @rsmith Would you prefer this (and http://reviews.llvm.org/D19842)

Re: [PATCH] D19843: Use the name of the file on disk to issue a new diagnostic about non-portable #include and #import paths.

eric_niebler updated the summary for this revision. eric_niebler updated this revision to Diff 59974. eric_niebler added a comment. 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

Re: [PATCH] D19843: Use the name of the file on disk to issue a new diagnostic about non-portable #include and #import paths.

twoh added a comment. Reverted in r271761. http://reviews.llvm.org/D19843 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D19843: Use the name of the file on disk to issue a new diagnostic about non-portable #include and #import paths.

twoh added a subscriber: twoh. twoh closed this revision. twoh added a comment. I have commit in r271708: http://reviews.llvm.org/rL271708 http://reviews.llvm.org/D19843 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

Re: [PATCH] D19843: Use the name of the file on disk to issue a new diagnostic about non-portable #include and #import paths.

eric_niebler updated this revision to Diff 59098. eric_niebler added a comment. Remove stale comment, tweak for diagnostic wording. http://reviews.llvm.org/D19843 Files: include/clang/Basic/DiagnosticGroups.td include/clang/Basic/DiagnosticLexKinds.td include/clang/Basic/FileManager.h

Re: [PATCH] D19843: Use the name of the file on disk to issue a new diagnostic about non-portable #include and #import paths.

eric_niebler added a comment. Awesome, thanks. Comment at: include/clang/Basic/DiagnosticLexKinds.td:278 @@ +277,3 @@ +def pp_nonportable_path : Warning< + "non-portable path '%0' found in preprocessor directive">, + InGroup; rsmith wrote: > The fix-it hints

Re: [PATCH] D19843: Use the name of the file on disk to issue a new diagnostic about non-portable #include and #import paths.

rsmith accepted this revision. rsmith added a reviewer: rsmith. rsmith added a comment. This revision is now accepted and ready to land. LGTM once the LLVM side is landed. Comment at: include/clang/Basic/DiagnosticLexKinds.td:278 @@ +277,3 @@ +def pp_nonportable_path : Warning<

Re: [PATCH] D19843: Use the name of the file on disk to issue a new diagnostic about non-portable #include and #import paths.

eric_niebler updated this revision to Diff 58828. eric_niebler added a comment. Add fixit tests, fix path separator fixit issue on Windows. http://reviews.llvm.org/D19843 Files: include/clang/Basic/DiagnosticGroups.td include/clang/Basic/DiagnosticLexKinds.td

Re: [PATCH] D19843: Use the name of the file on disk to issue a new diagnostic about non-portable #include and #import paths.

eric_niebler added a comment. > Please add some tests for the FixItHint replacement text. See test/FixIt for > examples of how to do this. Thanks for the suggestion. After adding tests for this, I noticed badness with the path separators in the FixIt hint when running on Windows. I'm working

Re: [PATCH] D19843: Use the name of the file on disk to issue a new diagnostic about non-portable #include and #import paths.

rsmith added a subscriber: rsmith. rsmith added a comment. Thanks for this! Sorry for the delay on the review side. Generally, the approach here looks fine, and I don't have any high-level concerns beyond a performance concern over whatever dark magic you're doing on the LLVM side to get this

Re: [PATCH] D19843: Use the name of the file on disk to issue a new diagnostic about non-portable #include and #import paths.

eric_niebler marked 2 inline comments as done. eric_niebler added a comment. 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? http://reviews.llvm.org/D19843

Re: [PATCH] D19843: Use the name of the file on disk to issue a new diagnostic about non-portable #include and #import paths.

curdeius added a comment. Nice job! Thanks for taking my remarks into account. Comment at: include/clang/Basic/VirtualFileSystem.h:14 Oops, my fault. http://reviews.llvm.org/D19843 ___ cfe-commits mailing list

Re: [PATCH] D19843: Use the name of the file on disk to issue a new diagnostic about non-portable #include and #import paths.

eric_niebler updated this revision to Diff 57657. eric_niebler added a comment. Factor out TrySimplifyPath from Preprocessor::HandleIncludeDirective. Other review feedback. http://reviews.llvm.org/D19843 Files: include/clang/Basic/DiagnosticGroups.td

Re: [PATCH] D19843: Use the name of the file on disk to issue a new diagnostic about non-portable #include and #import paths.

eric_niebler added inline comments. Comment at: include/clang/Basic/VirtualFileSystem.h:97 @@ +96,3 @@ + return Status->getName(); +else + return Status.getError(); curdeius wrote: > No else needed after return. But then `Status` is not in scope.

Re: [PATCH] D19843: Use the name of the file on disk to issue a new diagnostic about non-portable #include and #import paths.

eric_niebler added a comment. "pi" clangclang "ng" http://reviews.llvm.org/D19843 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D19843: Use the name of the file on disk to issue a new diagnostic about non-portable #include and #import paths.

eric_niebler created this revision. eric_niebler added a reviewer: bruno. eric_niebler added a subscriber: cfe-commits. Full discussion of this diff can be found here: http://lists.llvm.org/pipermail/cfe-dev/2016-April/048298.html See D19842 for the corresponding LLVM diff. Before this is