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.

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.

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.

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.

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.

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: r271708 - Use the name of the file on disk to issue a new diagnostic about non-portable #include and #import paths.

On 6/8/16, 11:12 AM, "tha...@google.com on behalf of Nico Weber" wrote: >On Wed, Jun 8, 2016 at 1:56 PM, Eric Niebler  wrote: >> >>(adding back cfe-commits, answers inline) >> >>On 6/8/16, 10:11 AM, "tha...@google.com on behalf

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

(adding back cfe-commits, answers inline) On 6/8/16, 10:11 AM, "tha...@google.com on behalf of Nico Weber" wrote: >Sounds like "commit to the current file case and fix all the world's includes" >then :-) (MinGW's windows headers have

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: r271708 - Use the name of the file on disk to issue a new diagnostic about non-portable #include and #import paths.

The information about whether the file is in a system path is already being computed, so it would incur no extra overhead. I'm not sure that's the right fix. You are more than welcome to revert the (clang) commit while we think of the right fix. \e On Jun 3, 2016 5:25 PM, "Bruno Cardoso Lopes"

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

On 6/3/16, 3:24 PM, "tha...@google.com on behalf of Nico Weber" wrote: > On Fri, Jun 3, 2016 at 6:14 PM, Eric Niebler wrote: >> I just checked, and warnings are not emitted from files in an -isystem path. >> I didn’t have to

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

I can investigate the system header issue. And I could add a special exception for if folks feel that that’s the right thing to do. If we’re really worried about how noisy this warning could be, the warning could be disabled by default. Thoughts? Eric On 6/3/16, 2:46 PM,

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

I just checked, and warnings are not emitted from files in an -isystem path. I didn’t have to do anything special to get that behavior. I don’t know about -imsvc. Is that a clang-cl thing? I can’t say at this point why the diagnostics system treats -isystem and -imsvc differently. How about

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.

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.

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.

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