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 via cfe-commits]

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
  include/clang/Lex/DirectoryLookup.h
  include/clang/Lex/HeaderSearch.h
  lib/Basic/FileManager.cpp
  lib/Basic/VirtualFileSystem.cpp
  lib/Lex/HeaderSearch.cpp
  lib/Lex/PPDirectives.cpp
  test/Lexer/Inputs/case-insensitive-include.h
  test/Lexer/case-insensitive-include-ms.c
  test/Lexer/case-insensitive-include.c
  test/Lexer/case-insensitive-system-include.c
  test/PCH/case-insensitive-include.c

Index: test/PCH/case-insensitive-include.c
===
--- test/PCH/case-insensitive-include.c
+++ test/PCH/case-insensitive-include.c
@@ -2,7 +2,7 @@
 
 // Test this without pch.
 // RUN: cp %S/Inputs/case-insensitive-include.h %T
-// RUN: %clang_cc1 -fsyntax-only %s -include %s -I %T -verify
+// RUN: %clang_cc1 -Wno-nonportable-include-path -fsyntax-only %s -include %s -I %T -verify
 
 // Test with pch.
 // RUN: %clang_cc1 -emit-pch -o %t.pch %s -I %T
Index: test/Lexer/case-insensitive-system-include.c
===
--- /dev/null
+++ test/Lexer/case-insensitive-system-include.c
@@ -0,0 +1,10 @@
+// REQUIRES: case-insensitive-filesystem
+
+// RUN: mkdir -p %T/asystempath
+// RUN: cp %S/Inputs/case-insensitive-include.h %T/asystempath/
+// RUN: cd %T
+// RUN: %clang_cc1 -fsyntax-only %s -include %s -isystem %T/asystempath -verify -Wnonportable-system-include-path
+// RUN: %clang_cc1 -fsyntax-only -fdiagnostics-parseable-fixits %s -include %s -isystem %T/asystempath -Wnonportable-system-include-path 2>&1 | FileCheck %s
+
+#include "CASE-INSENSITIVE-INCLUDE.H" // expected-warning {{non-portable path}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:38}:"\"case-insensitive-include.h\""
Index: test/Lexer/case-insensitive-include.c
===
--- /dev/null
+++ test/Lexer/case-insensitive-include.c
@@ -0,0 +1,35 @@
+// REQUIRES: case-insensitive-filesystem
+
+// RUN: mkdir -p %T/apath
+// RUN: mkdir -p %T/asystempath
+// RUN: cp %S/Inputs/case-insensitive-include.h %T
+// RUN: cp %S/Inputs/case-insensitive-include.h %T/asystempath/case-insensitive-include2.h
+// RUN: cd %T
+// RUN: %clang_cc1 -fsyntax-only %s -include %s -I %T -isystem %T/asystempath -verify
+// RUN: %clang_cc1 -fsyntax-only -fdiagnostics-parseable-fixits %s -include %s -I %T -isystem %T/asystempath 2>&1 | FileCheck %s
+
+// Known standard header, so warn:
+#include  // expected-warning {{non-portable path}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:20}:""
+
+#include "case-insensitive-include.h"
+#include "Case-Insensitive-Include.h" // expected-warning {{non-portable path}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:38}:"\"case-insensitive-include.h\""
+
+#include "../Output/./case-insensitive-include.h"
+#include "../Output/./Case-Insensitive-Include.h" // expected-warning {{non-portable path}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:50}:"\"../Output/./case-insensitive-include.h\""
+#include "../output/./case-insensitive-include.h" // expected-warning {{non-portable path}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:50}:"\"../Output/./case-insensitive-include.h\""
+
+#include "apath/.././case-insensitive-include.h"
+#include "apath/.././Case-Insensitive-Include.h" // expected-warning {{non-portable path}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:49}:"\"apath/.././case-insensitive-include.h\""
+#include "APath/.././case-insensitive-include.h" // For the sake of efficiency, this case is not diagnosed. :-(
+
+#include "../Output/./apath/.././case-insensitive-include.h"
+#include "../Output/./APath/.././case-insensitive-include.h" // For the sake of efficiency, this case is not diagnosed. :-(
+#include "../output/./apath/.././case-insensitive-include.h" // expected-warning {{non-portable path}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:61}:"\"../Output/./apath/.././case-insensitive-include.h\""
+
+#include "CASE-INSENSITIVE-INCLUDE2.H" // Found in an -isystem directory. No warning.
Index: test/Lexer/case-insensitive-include-ms.c
===
--- /dev/null
+++ test/Lexer/case-insensitive-include-ms.c
@@ -0,0 +1,18 @@
+// REQUIRES: case-insensitive-filesystem
+
+// RUN: mkdir -p %T/apath
+// RUN: cp %S/Inputs/case-insensitive-include.h %T
+// RUN: cd %T
+// RUN: %clang_cc1 -fsyntax-only -fms-compatibility %s -include %s -I %T -verify
+// RUN: %clang_cc1 -fsyntax-only -fms-compatibility -fdiagnostics-parseable-fixits %s -include %s -I %T 2>&1 | FileCheck %s
+
+#include "..\Output\.\case-insensitive-include.h"
+#include "..\Output\.\Case-Insensitive-Include.h" // expected-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 via cfe-commits]

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



___
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 via cfe-commits]

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
  include/clang/Basic/VirtualFileSystem.h
  include/clang/Lex/DirectoryLookup.h
  include/clang/Lex/HeaderSearch.h
  lib/Basic/FileManager.cpp
  lib/Basic/VirtualFileSystem.cpp
  lib/Lex/HeaderSearch.cpp
  lib/Lex/PPDirectives.cpp
  test/Lexer/Inputs/case-insensitive-include.h
  test/Lexer/case-insensitive-include-ms.c
  test/Lexer/case-insensitive-include.c
  test/Lexer/case-insensitive-system-include.c
  test/PCH/case-insensitive-include.c

Index: test/PCH/case-insensitive-include.c
===
--- test/PCH/case-insensitive-include.c
+++ test/PCH/case-insensitive-include.c
@@ -2,7 +2,7 @@
 
 // Test this without pch.
 // RUN: cp %S/Inputs/case-insensitive-include.h %T
-// RUN: %clang_cc1 -fsyntax-only %s -include %s -I %T -verify
+// RUN: %clang_cc1 -Wno-nonportable-include-path -fsyntax-only %s -include %s -I %T -verify
 
 // Test with pch.
 // RUN: %clang_cc1 -emit-pch -o %t.pch %s -I %T
Index: test/Lexer/case-insensitive-system-include.c
===
--- /dev/null
+++ test/Lexer/case-insensitive-system-include.c
@@ -0,0 +1,10 @@
+// REQUIRES: case-insensitive-filesystem
+
+// RUN: mkdir -p %T/asystempath
+// RUN: cp %S/Inputs/case-insensitive-include.h %T/asystempath/
+// RUN: cd %T
+// RUN: %clang_cc1 -fsyntax-only %s -include %s -isystem %T/asystempath -verify -Wnonportable-system-include-path
+// RUN: %clang_cc1 -fsyntax-only -fdiagnostics-parseable-fixits %s -include %s -isystem %T/asystempath -Wnonportable-system-include-path 2>&1 | FileCheck %s
+
+#include "CASE-INSENSITIVE-INCLUDE.H" // expected-warning {{non-portable path}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:38}:"\"case-insensitive-include.h\""
Index: test/Lexer/case-insensitive-include.c
===
--- /dev/null
+++ test/Lexer/case-insensitive-include.c
@@ -0,0 +1,35 @@
+// REQUIRES: case-insensitive-filesystem
+
+// RUN: mkdir -p %T/apath
+// RUN: mkdir -p %T/asystempath
+// RUN: cp %S/Inputs/case-insensitive-include.h %T
+// RUN: cp %S/Inputs/case-insensitive-include.h %T/asystempath/case-insensitive-include2.h
+// RUN: cd %T
+// RUN: %clang_cc1 -fsyntax-only %s -include %s -I %T -isystem %T/asystempath -verify
+// RUN: %clang_cc1 -fsyntax-only -fdiagnostics-parseable-fixits %s -include %s -I %T -isystem %T/asystempath 2>&1 | FileCheck %s
+
+// Known standard header, so warn:
+#include  // expected-warning {{non-portable path}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:20}:""
+
+#include "case-insensitive-include.h"
+#include "Case-Insensitive-Include.h" // expected-warning {{non-portable path}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:38}:"\"case-insensitive-include.h\""
+
+#include "../Output/./case-insensitive-include.h"
+#include "../Output/./Case-Insensitive-Include.h" // expected-warning {{non-portable path}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:50}:"\"../Output/./case-insensitive-include.h\""
+#include "../output/./case-insensitive-include.h" // expected-warning {{non-portable path}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:50}:"\"../Output/./case-insensitive-include.h\""
+
+#include "apath/.././case-insensitive-include.h"
+#include "apath/.././Case-Insensitive-Include.h" // expected-warning {{non-portable path}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:49}:"\"apath/.././case-insensitive-include.h\""
+#include "APath/.././case-insensitive-include.h" // For the sake of efficiency, this case is not diagnosed. :-(
+
+#include "../Output/./apath/.././case-insensitive-include.h"
+#include "../Output/./APath/.././case-insensitive-include.h" // For the sake of efficiency, this case is not diagnosed. :-(
+#include "../output/./apath/.././case-insensitive-include.h" // expected-warning {{non-portable path}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:61}:"\"../Output/./apath/.././case-insensitive-include.h\""
+
+#include "CASE-INSENSITIVE-INCLUDE2.H" // Found in an -isystem directory. No warning.
Index: test/Lexer/case-insensitive-include-ms.c
===
--- /dev/null
+++ test/Lexer/case-insensitive-include-ms.c
@@ -0,0 +1,18 @@
+// REQUIRES: case-insensitive-filesystem
+
+// RUN: mkdir -p %T/apath
+// RUN: cp %S/Inputs/case-insensitive-include.h %T
+// RUN: cd %T
+// RUN: %clang_cc1 -fsyntax-only -fms-compatibility %s -include %s -I %T -verify
+// RUN: %clang_cc1 -fsyntax-only -fms-compatibility -fdiagnostics-parseable-fixits %s -include %s -I %T 2>&1 | FileCheck %s
+
+#include "..\Output\.\case-insensitive-include.h"
+#include 

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 via cfe-commits]

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)?


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 via cfe-commits]

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 isn't ours.


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 via cfe-commits]

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) 
`StringSet`, and 3) noop. I found no measurable performance difference between 
any of them. This code path just isn't hot enough to matter.

If I don't hear back, maybe @twoh will merge these diffs again by EOD. And 
hopefully we'll hear fewer screams this time. :-)


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 via cfe-commits]

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
  include/clang/Basic/FileManager.h
  include/clang/Basic/VirtualFileSystem.h
  include/clang/Lex/DirectoryLookup.h
  include/clang/Lex/HeaderSearch.h
  lib/Basic/FileManager.cpp
  lib/Basic/VirtualFileSystem.cpp
  lib/Lex/HeaderSearch.cpp
  lib/Lex/PPDirectives.cpp
  test/Lexer/Inputs/case-insensitive-include.h
  test/Lexer/case-insensitive-include-ms.c
  test/Lexer/case-insensitive-include.c
  test/Lexer/case-insensitive-system-include.c
  test/PCH/case-insensitive-include.c

Index: test/PCH/case-insensitive-include.c
===
--- test/PCH/case-insensitive-include.c
+++ test/PCH/case-insensitive-include.c
@@ -2,7 +2,7 @@
 
 // Test this without pch.
 // RUN: cp %S/Inputs/case-insensitive-include.h %T
-// RUN: %clang_cc1 -fsyntax-only %s -include %s -I %T -verify
+// RUN: %clang_cc1 -Wno-nonportable-include-path -fsyntax-only %s -include %s -I %T -verify
 
 // Test with pch.
 // RUN: %clang_cc1 -emit-pch -o %t.pch %s -I %T
Index: test/Lexer/case-insensitive-system-include.c
===
--- /dev/null
+++ test/Lexer/case-insensitive-system-include.c
@@ -0,0 +1,10 @@
+// REQUIRES: case-insensitive-filesystem
+
+// RUN: mkdir -p %T/asystempath
+// RUN: cp %S/Inputs/case-insensitive-include.h %T/asystempath/
+// RUN: cd %T
+// RUN: %clang_cc1 -fsyntax-only %s -include %s -isystem %T/asystempath -verify -Wnonportable-system-include-path
+// RUN: %clang_cc1 -fsyntax-only -fdiagnostics-parseable-fixits %s -include %s -isystem %T/asystempath -Wnonportable-system-include-path 2>&1 | FileCheck %s
+
+#include "CASE-INSENSITIVE-INCLUDE.H" // expected-warning {{non-portable path}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:38}:"\"case-insensitive-include.h\""
Index: test/Lexer/case-insensitive-include.c
===
--- /dev/null
+++ test/Lexer/case-insensitive-include.c
@@ -0,0 +1,35 @@
+// REQUIRES: case-insensitive-filesystem
+
+// RUN: mkdir -p %T/apath
+// RUN: mkdir -p %T/asystempath
+// RUN: cp %S/Inputs/case-insensitive-include.h %T
+// RUN: cp %S/Inputs/case-insensitive-include.h %T/asystempath/case-insensitive-include2.h
+// RUN: cd %T
+// RUN: %clang_cc1 -fsyntax-only %s -include %s -I %T -isystem %T/asystempath -verify
+// RUN: %clang_cc1 -fsyntax-only -fdiagnostics-parseable-fixits %s -include %s -I %T -isystem %T/asystempath 2>&1 | FileCheck %s
+
+// Known standard header, so warn:
+#include  // expected-warning {{non-portable path}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:20}:""
+
+#include "case-insensitive-include.h"
+#include "Case-Insensitive-Include.h" // expected-warning {{non-portable path}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:38}:"\"case-insensitive-include.h\""
+
+#include "../Output/./case-insensitive-include.h"
+#include "../Output/./Case-Insensitive-Include.h" // expected-warning {{non-portable path}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:50}:"\"../Output/./case-insensitive-include.h\""
+#include "../output/./case-insensitive-include.h" // expected-warning {{non-portable path}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:50}:"\"../Output/./case-insensitive-include.h\""
+
+#include "apath/.././case-insensitive-include.h"
+#include "apath/.././Case-Insensitive-Include.h" // expected-warning {{non-portable path}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:49}:"\"apath/.././case-insensitive-include.h\""
+#include "APath/.././case-insensitive-include.h" // For the sake of efficiency, this case is not diagnosed. :-(
+
+#include "../Output/./apath/.././case-insensitive-include.h"
+#include "../Output/./APath/.././case-insensitive-include.h" // For the sake of efficiency, this case is not diagnosed. :-(
+#include "../output/./apath/.././case-insensitive-include.h" // expected-warning {{non-portable path}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:61}:"\"../Output/./apath/.././case-insensitive-include.h\""
+
+#include "CASE-INSENSITIVE-INCLUDE2.H" // Found in an -isystem directory. No warning.
Index: test/Lexer/case-insensitive-include-ms.c
===
--- /dev/null
+++ test/Lexer/case-insensitive-include-ms.c
@@ -0,0 +1,18 @@
+// REQUIRES: case-insensitive-filesystem
+
+// RUN: mkdir -p %T/apath
+// RUN: cp %S/Inputs/case-insensitive-include.h %T
+// RUN: cd %T
+// RUN: %clang_cc1 -fsyntax-only -fms-compatibility %s -include %s -I %T -verify
+// RUN: %clang_cc1 -fsyntax-only -fms-compatibility -fdiagnostics-parseable-fixits %s -include %s -I %T 2>&1 | FileCheck %s
+
+#include 

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 via cfe-commits]

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 `StringSwitch` just 
> > turns into cascading `memcmp`'s. Bet I can tell you how that performs 
> > versus a hash set. :-) Also, with the `StringSet`, I get to initialize it 
> > once and reuse it many times. I expect that will be pretty darn quick at 
> > runtime, but I'm looking forward to @bruno's results.
> Right, I'm not suggesting `StringSwitch` will be faster; it's preferable for 
> other reasons (it avoids the memory and shutdown costs of the static local 
> set). We should stick with what you have if the performance advantage is 
> measurable; otherwise my preference would be to use `StringSwitch`. But it's 
> only a slight preference -- if you'd rather not, I won't complain.
> 
> [`StringSwitch` isn't /quite/ as bad as you're suggesting: it always first 
> compares on length, and it typically compiles into a switch on string length 
> followed by memcmps. Moreover, the code should be "obvious" enough that 
> compilers can (at least in principle) optimize those memcmps very 
> aggressively, right down into the equivalent of an unrolled DFA or a perfect 
> hash function, but I'm not at all confident that LLVM will actually do that 
> =)]
@rsmith wrote:
> We should stick with what you have if the performance advantage is 
> measurable; otherwise my preference would be to use `StringSwitch`.

I tried `StringSwitch` on Windows.h where there are lots of wrongly-cased 
#includes. I couldn't measure the difference. I'll update the diff. 


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 via cfe-commits]

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 that path 
-- at least not on my machine. As a result, the attempt to get the "real path 
name" fails and no diagnostic is issued; the code path you want to measure will 
never be hit. This appears to be a shortcoming of the way I'm trying to get the 
true case of the files as they are opened. Correctly handing this would greatly 
increase the complexity of the patch. Apple == :-(


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 via cfe-commits]

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 at 0x7F.
This should be `0x7F`. I'll fix it.


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 via cfe-commits]

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 cascading `memcmp`'s. Bet I can tell you how that performs versus a 
hash set. :-) Also, with the `StringSet`, I get to initialize it once and reuse 
it many times. I expect that will be pretty darn quick at runtime, but I'm 
looking forward to @bruno's results.


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

[Eric Niebler via cfe-commits]

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 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 different case than MSVC's, 
>>>making this extra annoying.)
>>>
>>>Maybe this warning could be keyed off -fmsc-version in clang-cl then and 
>>>only be enabled there once a future MSVC with headers where this is fixed 
>>>has been deployed and is picked via -fmsc-version?
>>
>>I just changed the patch so that #includes to headers found in system include 
>>paths do not trigger this warning by default (unless the header is a known 
>>“portable” header – i.e., standard C/C++/Posix or Boost headers). This 
>>shouldn’t be an issue anymore.
>
>Well, it'd be nice if the warning would fire everywhere and everyone got their 
>cases right – then a project that builds on a case-insensitive file system 
>should compile fine if it's moved to a case-sensitive one.

Sure, that’d be nice. And folks who really care can get that with my patch by 
flipping –Wnonportable-system-include-path; it’s just not the default. You 
could think of this patch as a staged rollout of the feature.

\e
 


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Eric Niebler via cfe-commits]

(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 different case than MSVC's, making this 
>extra annoying.) 
>
>Maybe this warning could be keyed off -fmsc-version in clang-cl then and only 
>be enabled there once a future MSVC with headers where this is fixed has been 
>deployed and is picked via -fmsc-version?

I just changed the patch so that #includes to headers found in system include 
paths do not trigger this warning by default (unless the header is a known 
“portable” header – i.e., standard C/C++/Posix or Boost headers). This 
shouldn’t be an issue anymore.

>Eric, do you think you could send Stephan a list of #includes in Windows 
>headers that would need changing?

Since warnings are not emitted from within system headers, and since 
badly-cased #includes of WinSDK headers will no longer warn by default, is this 
still an issue? I don’t have an easy way to generate this list.

>(any reason cfe-commits got dropped from the cc list? This would be good to 
>discuss on-thread.)
>
>On Tue, Jun 7, 2016 at 6:42 PM, Stephan T. Lavavej 
> wrote:
>>Knowing Windows, there is probably no way that they would change the case on 
>>disk on the headers, as that would be observable to directory enumeration.
>> 
>>STL
>> 
>>From: tha...@google.com [mailto:tha...@google.com] On Behalf Of Nico Weber
>>Sent: Tuesday, June 7, 2016 3:40 PM
>>To: Stephan T. Lavavej 
>>Cc: Eric Niebler ; Taewook Oh 
>>Subject: Re: r271708 - Use the name of the file on disk to issue a new 
>>diagnostic about non-portable #include and #import paths.
>> 
>>>STL: "Make sure all filenames have lower-case names and all #includes refer 
>>>to them by lower-case name, and tell all users to include files by lower 
>>>case" is probably the only SDK change that's simple enough that it has a 
>>>chance to be understood and to actually happen. (The other alternative is 
>>"commit to keep the case of each SDK header as-is forever, and ask every 
>>>client to use the on-disk casing, which unless they use a compiler with a 
>>>warning like this they'll have to look up themselves" -- and at the moment I 
>>>think the on-disk case of windows.h is Windows.h while every program I 
>>know of includes it as , so this would require changing 
>>>decades of existing code.)
>>> 
>>>On Tue, Jun 7, 2016 at 12:57 PM, Stephan T. Lavavej 
>>> wrote:
Sure, but it's time consuming to figure out a build, and I have a limited 
amount of time to donate.

On the other hand, if this gets into trunk and eventually Clang/C2, at that 
point auditing the headers would be pretty easy over here.

STL 

-Original Message-
From: Eric Niebler [mailto:enieb...@fb.com]
Sent: Tuesday, June 7, 2016 9:00 AM
To: Stephan T. Lavavej ; Nico Weber 

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

>Someone who builds and distributes their own mingw distribution can figure 
>out how to build clang. ☺ The directions are here: 
>http://clang.llvm.org/get_started.html. I’m happy to answer any questions.
>
>\e
>
>On 6/6/16, 5:54 PM, "Stephan T. Lavavej"  
>wrote:
>>Unfortunately, I haven't figured out how to build Clang/LLVM (all of my 
>>Clang work is with Clang/C2 which they build for me).
>>
>>STL
>>
>>-Original Message-
>>From: Eric Niebler [mailto:enieb...@fb.com]
>>Sent: Monday, June 6, 2016 3:19 PM
>>To: Stephan T. Lavavej ; Nico Weber 
>>
>>Cc: Taewook Oh 
>>Subject: 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, 5:17 PM, "Stephan T. Lavavej"  
>>wrote:
>>> [Eric Niebler]
 Once I sort out the -imsvc thing, how bad would it be to leave it 
 alone?
 Probably pretty bad for folks in the Windows world, huh?
>>>
>>> If you can send me an explicit list of WinSDK headers that are 
>>> including things with the wrong case, I can send it to Windows - they 
>>> are committed to cleaning up the WinSDK in order to make it friendlier 
>>> to other tools.
>>>
>>> Note that we can't go back in time and fix the shipped SDKs, though.
>>
>>It would actually be easier for you to generate this list than for me 
>>since I’m not set up to 

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 via cfe-commits]

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) in new diffs?

@bruno, @bogner: This should fix the use-after-free that ASAN was complaining 
about. ASAN rules.


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 via cfe-commits]

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 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 . (As always, warning 
//within// system headers are suppressed.)


http://reviews.llvm.org/D19843

Files:
  include/clang/Basic/DiagnosticLexKinds.td
  include/clang/Basic/FileManager.h
  include/clang/Basic/VirtualFileSystem.h
  include/clang/Lex/DirectoryLookup.h
  include/clang/Lex/HeaderSearch.h
  lib/Basic/FileManager.cpp
  lib/Basic/VirtualFileSystem.cpp
  lib/Lex/HeaderSearch.cpp
  lib/Lex/PPDirectives.cpp
  test/Lexer/Inputs/case-insensitive-include.h
  test/Lexer/case-insensitive-include-ms.c
  test/Lexer/case-insensitive-include.c
  test/Lexer/case-insensitive-system-include.c
  test/PCH/case-insensitive-include.c

Index: test/PCH/case-insensitive-include.c
===
--- test/PCH/case-insensitive-include.c
+++ test/PCH/case-insensitive-include.c
@@ -2,7 +2,7 @@
 
 // Test this without pch.
 // RUN: cp %S/Inputs/case-insensitive-include.h %T
-// RUN: %clang_cc1 -fsyntax-only %s -include %s -I %T -verify
+// RUN: %clang_cc1 -Wno-nonportable-include-path -fsyntax-only %s -include %s -I %T -verify
 
 // Test with pch.
 // RUN: %clang_cc1 -emit-pch -o %t.pch %s -I %T
Index: test/Lexer/case-insensitive-system-include.c
===
--- test/Lexer/case-insensitive-system-include.c
+++ test/Lexer/case-insensitive-system-include.c
@@ -0,0 +1,10 @@
+// REQUIRES: case-insensitive-filesystem
+
+// RUN: mkdir -p %T/asystempath
+// RUN: cp %S/Inputs/case-insensitive-include.h %T/asystempath/
+// RUN: cd %T
+// RUN: %clang_cc1 -fsyntax-only %s -include %s -isystem %T/asystempath -verify -Wnonportable-system-include-path
+// RUN: %clang_cc1 -fsyntax-only -fdiagnostics-parseable-fixits %s -include %s -isystem %T/asystempath -Wnonportable-system-include-path 2>&1 | FileCheck %s
+
+#include "CASE-INSENSITIVE-INCLUDE.H" // expected-warning {{non-portable path}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:38}:"\"case-insensitive-include.h\""
Index: test/Lexer/case-insensitive-include.c
===
--- test/Lexer/case-insensitive-include.c
+++ test/Lexer/case-insensitive-include.c
@@ -0,0 +1,35 @@
+// REQUIRES: case-insensitive-filesystem
+
+// RUN: mkdir -p %T/apath
+// RUN: mkdir -p %T/asystempath
+// RUN: cp %S/Inputs/case-insensitive-include.h %T
+// RUN: cp %S/Inputs/case-insensitive-include.h %T/asystempath/case-insensitive-include2.h
+// RUN: cd %T
+// RUN: %clang_cc1 -fsyntax-only %s -include %s -I %T -isystem %T/asystempath -verify
+// RUN: %clang_cc1 -fsyntax-only -fdiagnostics-parseable-fixits %s -include %s -I %T -isystem %T/asystempath 2>&1 | FileCheck %s
+
+// Known standard header, so warn:
+#include  // expected-warning {{non-portable path}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:20}:""
+
+#include "case-insensitive-include.h"
+#include "Case-Insensitive-Include.h" // expected-warning {{non-portable path}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:38}:"\"case-insensitive-include.h\""
+
+#include "../Output/./case-insensitive-include.h"
+#include "../Output/./Case-Insensitive-Include.h" // expected-warning {{non-portable path}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:50}:"\"../Output/./case-insensitive-include.h\""
+#include "../output/./case-insensitive-include.h" // expected-warning {{non-portable path}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:50}:"\"../Output/./case-insensitive-include.h\""
+
+#include "apath/.././case-insensitive-include.h"
+#include "apath/.././Case-Insensitive-Include.h" // expected-warning {{non-portable path}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:49}:"\"apath/.././case-insensitive-include.h\""
+#include "APath/.././case-insensitive-include.h" // For the sake of efficiency, this case is not diagnosed. :-(
+
+#include "../Output/./apath/.././case-insensitive-include.h"
+#include "../Output/./APath/.././case-insensitive-include.h" // For the sake of efficiency, this case is not diagnosed. :-(
+#include "../output/./apath/.././case-insensitive-include.h" // expected-warning {{non-portable path}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:61}:"\"../Output/./apath/.././case-insensitive-include.h\""
+
+#include "CASE-INSENSITIVE-INCLUDE2.H" // Found in an -isystem directory. No warning.
Index: test/Lexer/case-insensitive-include-ms.c
===
--- test/Lexer/case-insensitive-include-ms.c
+++ 

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

[Eric Niebler via cfe-commits]

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" 
wrote:

I think this should be reverted until we figure out how to solve it.
Moreover, it looks expensive to check for each file if it's within a system
path. I would really like to measure compile time for this change before it
goes definitely in.

On Fri, Jun 3, 2016 at 4:53 PM, Justin Bogner via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> Taewook Oh via cfe-commits  writes:
> > Author: twoh
> > Date: Fri Jun  3 13:52:51 2016
> > New Revision: 271708
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=271708=rev
> > Log:
> > Use the name of the file on disk to issue a new diagnostic about
> > non-portable #include and #import paths.
> >
> > Differential Revision: http://reviews.llvm.org/D19843
> > Corresponding LLVM change: http://reviews.llvm.org/D19842
> >
> > Patch by Eric Niebler
> >
> >
> > Added:
> > cfe/trunk/test/Lexer/Inputs/case-insensitive-include.h
> > cfe/trunk/test/Lexer/case-insensitive-include-ms.c
> > cfe/trunk/test/Lexer/case-insensitive-include.c
> > Modified:
> > cfe/trunk/include/clang/Basic/DiagnosticGroups.td
> > cfe/trunk/include/clang/Basic/DiagnosticLexKinds.td
> > cfe/trunk/include/clang/Basic/FileManager.h
> > cfe/trunk/include/clang/Basic/VirtualFileSystem.h
> > cfe/trunk/include/clang/Lex/DirectoryLookup.h
> > cfe/trunk/include/clang/Lex/HeaderSearch.h
> > cfe/trunk/lib/Basic/FileManager.cpp
> > cfe/trunk/lib/Basic/VirtualFileSystem.cpp
> > cfe/trunk/lib/Lex/HeaderSearch.cpp
> > cfe/trunk/lib/Lex/PPDirectives.cpp
> > cfe/trunk/test/PCH/case-insensitive-include.c
> >
> > Modified: cfe/trunk/include/clang/Basic/DiagnosticGroups.td
> > URL:
> >
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticGroups.td?rev=271708=271707=271708=diff
> >
> ==
> >
> > --- cfe/trunk/include/clang/Basic/DiagnosticGroups.td (original)
> > +++ cfe/trunk/include/clang/Basic/DiagnosticGroups.td Fri Jun  3
> 13:52:51 2016
> > @@ -390,6 +390,7 @@ def : DiagGroup<"sequence-point", [Unseq
> >  def AmbiguousMacro : DiagGroup<"ambiguous-macro">;
> >  def KeywordAsMacro : DiagGroup<"keyword-macro">;
> >  def ReservedIdAsMacro : DiagGroup<"reserved-id-macro">;
> > +def NonportableIncludePath : DiagGroup<"nonportable-include-path">;
> >
> >  // Just silence warnings about -Wstrict-aliasing for now.
> >  def : DiagGroup<"strict-aliasing=0">;
> >
> > Modified: cfe/trunk/include/clang/Basic/DiagnosticLexKinds.td
> > URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticLexKinds.td?rev=271708=271707=271708=diff
> >
> ==
> > --- cfe/trunk/include/clang/Basic/DiagnosticLexKinds.td (original)
> > +++ cfe/trunk/include/clang/Basic/DiagnosticLexKinds.td Fri Jun  3
> 13:52:51 2016
> > @@ -274,6 +274,10 @@ def ext_missing_whitespace_after_macro_n
> >"whitespace required after macro name">;
> >  def warn_missing_whitespace_after_macro_name : Warning<
> >"whitespace recommended after macro name">;
> > +def pp_nonportable_path : Warning<
> > +  "non-portable path to file '%0'; specified path differs in case from
> file"
> > +  " name on disk">,
> > +  InGroup;
> >
> >  def pp_pragma_once_in_main_file : Warning<"#pragma once in main file">,
> >InGroup>;
> >
> > Modified: cfe/trunk/include/clang/Basic/FileManager.h
> > URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/FileManager.h?rev=271708=271707=271708=diff
> >
> ==
> > --- cfe/trunk/include/clang/Basic/FileManager.h (original)
> > +++ cfe/trunk/include/clang/Basic/FileManager.h Fri Jun  3 13:52:51 2016
> > @@ -52,6 +52,7 @@ public:
> >  /// descriptor for the file.
> >  class FileEntry {
> >const char *Name;   // Name of the file.
> > +  std::string RealPathName;   // Real path to the file; could be empty.
> >off_t Size; // File size in bytes.
> >time_t ModTime; // Modification time of file.
> >const DirectoryEntry *Dir;  // Directory file lives in.
> > @@ -82,6 +83,7 @@ public:
> >}
> >
> >const char *getName() const { return Name; }
> > +  StringRef tryGetRealPathName() const { return RealPathName; }
> >bool isValid() const { return IsValid; }
> >off_t getSize() const { return Size; }
> >unsigned getUID() const { return UID; }
> >
> > Modified: cfe/trunk/include/clang/Basic/VirtualFileSystem.h
> > URL:
> 

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

[Eric Niebler via cfe-commits]

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 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 this: I can change the diagnostic to not warn about filenames if 
>> the file is found in a system include path, regardless of
>> the location of the #include directive.
>
> That sounds like a good idea to me!

On second thought, this warning isn’t living up to its potential if it doesn’t 
warn on `#include `. And if we don’t help people find misspellings of 
IOKIt, we haven’t done much good at all.

Once I sort out the -imsvc thing, how bad would it be to leave it alone? 
Probably pretty bad for folks in the Windows world, huh?

\e







___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Eric Niebler via cfe-commits]

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, "tha...@google.com on behalf of 
Nico Weber"  on behalf of 
tha...@chromium.org> wrote:

Also, once that is resolved, this warning tells people that #include 
 is wrong:

..\..\third_party\ffmpeg\compat/w32pthreads.h(39,10):  warning: non-portable 
path to file ''; specified path differs in case from file name on 
disk [-Wnonportable-include-path]
#include 
 ^~~
 

I don't think that's realistically fixable; about all the code in the world 
includes windows.h as windows.h...any ideas what the Windows story for this 
warning should be?

On Fri, Jun 3, 2016 at 5:43 PM, Nico Weber 
> wrote:
I'm getting lots of warnings like so:

In file included from 
..\..\tools\win\static_initializers\static_initializers.cc:5:
C:\b\depot_tools\win_toolchain\vs_files\95ddda401ec5678f15eeed01d2bee08fcbc5ee97\DIA
 SDK\include\dia2.h(30,10):  error: non-portable path to file '"Windows.h"'; 
specified path differs in case from file name on disk 
[-Werror,-Wnonportable-include-path]
#include "windows.h"
 ^~~
 "Windows.h"

dia2.h is a system include, and we include it in the search path via -imsvc. 
Normally warnings aren't emitted for system headers. Do you know why this is 
happening here?

On Fri, Jun 3, 2016 at 2:52 PM, Taewook Oh via cfe-commits 
> wrote:
Author: twoh
Date: Fri Jun  3 13:52:51 2016
New Revision: 271708

URL: 
http://llvm.org/viewvc/llvm-project?rev=271708=rev
Log:
Use the name of the file on disk to issue a new diagnostic about non-portable 
#include and #import paths.

Differential Revision: 
http://reviews.llvm.org/D19843
Corresponding LLVM change: 
http://reviews.llvm.org/D19842

Patch by Eric Niebler


Added:
cfe/trunk/test/Lexer/Inputs/case-insensitive-include.h
cfe/trunk/test/Lexer/case-insensitive-include-ms.c
cfe/trunk/test/Lexer/case-insensitive-include.c
Modified:
cfe/trunk/include/clang/Basic/DiagnosticGroups.td
cfe/trunk/include/clang/Basic/DiagnosticLexKinds.td
cfe/trunk/include/clang/Basic/FileManager.h
cfe/trunk/include/clang/Basic/VirtualFileSystem.h
cfe/trunk/include/clang/Lex/DirectoryLookup.h
cfe/trunk/include/clang/Lex/HeaderSearch.h
cfe/trunk/lib/Basic/FileManager.cpp
cfe/trunk/lib/Basic/VirtualFileSystem.cpp
cfe/trunk/lib/Lex/HeaderSearch.cpp
cfe/trunk/lib/Lex/PPDirectives.cpp
cfe/trunk/test/PCH/case-insensitive-include.c

Modified: cfe/trunk/include/clang/Basic/DiagnosticGroups.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticGroups.td?rev=271708=271707=271708=diff
==
--- cfe/trunk/include/clang/Basic/DiagnosticGroups.td (original)
+++ cfe/trunk/include/clang/Basic/DiagnosticGroups.td Fri Jun  3 13:52:51 2016
@@ -390,6 +390,7 @@ def : DiagGroup<"sequence-point", [Unseq
 def AmbiguousMacro : DiagGroup<"ambiguous-macro">;
 def KeywordAsMacro : DiagGroup<"keyword-macro">;
 def ReservedIdAsMacro : DiagGroup<"reserved-id-macro">;
+def NonportableIncludePath : DiagGroup<"nonportable-include-path">;

 // Just silence warnings about -Wstrict-aliasing for now.
 def : DiagGroup<"strict-aliasing=0">;

Modified: cfe/trunk/include/clang/Basic/DiagnosticLexKinds.td
URL: 

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

[Eric Niebler via cfe-commits]

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 this: I can change the diagnostic to not warn about filenames if the 
file is found in a system include path, regardless of the location of the 
#include directive. Other than that, I can whitelist specific filenames, but 
that quickly becomes a game of whack-a-mole.

Eric


On 6/3/16, 3:01 PM, "tha...@google.com on behalf of 
Nico Weber"  on behalf of 
tha...@chromium.org> wrote:

It's not just windows.h, but windows sdk headers in general (#include 
, #include  #include  #include  
etc). Here's what the warning does on the first few files in Chromium: 
https://build.chromium.org/p/chromium.fyi/builders/ClangToTWin/builds/8155/steps/compile/logs/stdio
 (We add /FIIntrin.h -- but it looks like MSVC's intrin.h is called intrin.h 
while clang's is called Intrin.h, which is probably a bug in clang).

I think in general clang's philosophy is that warnings should be useful enough 
that they should be able to be on by default, so not having it on by default 
should be some last resort. Better to try and make it work well enough that it 
can be enabled :-)




On Fri, Jun 3, 2016 at 5:54 PM, Eric Niebler 
> wrote:
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, "tha...@google.com on behalf of 
Nico Weber"  on behalf of 
tha...@chromium.org> wrote:

Also, once that is resolved, this warning tells people that #include 
 is wrong:

..\..\third_party\ffmpeg\compat/w32pthreads.h(39,10):  warning: non-portable 
path to file ''; specified path differs in case from file name on 
disk [-Wnonportable-include-path]
#include 
 ^~~
 

I don't think that's realistically fixable; about all the code in the world 
includes windows.h as windows.h...any ideas what the Windows story for this 
warning should be?

On Fri, Jun 3, 2016 at 5:43 PM, Nico Weber 
> wrote:
I'm getting lots of warnings like so:

In file included from 
..\..\tools\win\static_initializers\static_initializers.cc:5:
C:\b\depot_tools\win_toolchain\vs_files\95ddda401ec5678f15eeed01d2bee08fcbc5ee97\DIA
 SDK\include\dia2.h(30,10):  error: non-portable path to file '"Windows.h"'; 
specified path differs in case from file name on disk 
[-Werror,-Wnonportable-include-path]
#include "windows.h"
 ^~~
 "Windows.h"

dia2.h is a system include, and we include it in the search path via -imsvc. 
Normally warnings aren't emitted for system headers. Do you know why this is 
happening here?

On Fri, Jun 3, 2016 at 2:52 PM, Taewook Oh via cfe-commits 
> wrote:
Author: twoh
Date: Fri Jun  3 13:52:51 2016
New Revision: 271708

URL: 
http://llvm.org/viewvc/llvm-project?rev=271708=rev
Log:
Use the name of the file on disk to issue a new diagnostic about non-portable 
#include and #import paths.

Differential Revision: 
http://reviews.llvm.org/D19843
Corresponding LLVM change: 
http://reviews.llvm.org/D19842

Patch by Eric Niebler


Added:
cfe/trunk/test/Lexer/Inputs/case-insensitive-include.h
cfe/trunk/test/Lexer/case-insensitive-include-ms.c
cfe/trunk/test/Lexer/case-insensitive-include.c
Modified:
cfe/trunk/include/clang/Basic/DiagnosticGroups.td
cfe/trunk/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 via cfe-commits]

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
  include/clang/Basic/VirtualFileSystem.h
  include/clang/Lex/DirectoryLookup.h
  include/clang/Lex/HeaderSearch.h
  lib/Basic/FileManager.cpp
  lib/Basic/VirtualFileSystem.cpp
  lib/Lex/HeaderSearch.cpp
  lib/Lex/PPDirectives.cpp
  test/Lexer/Inputs/case-insensitive-include.h
  test/Lexer/case-insensitive-include-ms.c
  test/Lexer/case-insensitive-include.c
  test/PCH/case-insensitive-include.c

Index: test/PCH/case-insensitive-include.c
===
--- test/PCH/case-insensitive-include.c
+++ test/PCH/case-insensitive-include.c
@@ -2,7 +2,7 @@
 
 // Test this without pch.
 // RUN: cp %S/Inputs/case-insensitive-include.h %T
-// RUN: %clang_cc1 -fsyntax-only %s -include %s -I %T -verify
+// RUN: %clang_cc1 -Wno-nonportable-include-path -fsyntax-only %s -include %s -I %T -verify
 
 // Test with pch.
 // RUN: %clang_cc1 -emit-pch -o %t.pch %s -I %T
Index: test/Lexer/case-insensitive-include.c
===
--- /dev/null
+++ test/Lexer/case-insensitive-include.c
@@ -0,0 +1,27 @@
+// REQUIRES: case-insensitive-filesystem
+
+// RUN: mkdir -p %T/apath
+// RUN: cp %S/Inputs/case-insensitive-include.h %T
+// RUN: cd %T
+// RUN: %clang_cc1 -fsyntax-only %s -include %s -I %T -verify
+// RUN: %clang_cc1 -fsyntax-only -fdiagnostics-parseable-fixits %s -include %s -I %T 2>&1 | FileCheck %s
+
+#include "case-insensitive-include.h"
+#include "Case-Insensitive-Include.h" // expected-warning {{non-portable path}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:38}:"\"case-insensitive-include.h\""
+
+#include "../Output/./case-insensitive-include.h"
+#include "../Output/./Case-Insensitive-Include.h" // expected-warning {{non-portable path}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:50}:"\"../Output/./case-insensitive-include.h\""
+#include "../output/./case-insensitive-include.h" // expected-warning {{non-portable path}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:50}:"\"../Output/./case-insensitive-include.h\""
+
+#include "apath/.././case-insensitive-include.h"
+#include "apath/.././Case-Insensitive-Include.h" // expected-warning {{non-portable path}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:49}:"\"apath/.././case-insensitive-include.h\""
+#include "APath/.././case-insensitive-include.h" // For the sake of efficiency, this case is not diagnosed. :-(
+
+#include "../Output/./apath/.././case-insensitive-include.h"
+#include "../Output/./APath/.././case-insensitive-include.h" // For the sake of efficiency, this case is not diagnosed. :-(
+#include "../output/./apath/.././case-insensitive-include.h" // expected-warning {{non-portable path}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:61}:"\"../Output/./apath/.././case-insensitive-include.h\""
Index: test/Lexer/case-insensitive-include-ms.c
===
--- /dev/null
+++ test/Lexer/case-insensitive-include-ms.c
@@ -0,0 +1,18 @@
+// REQUIRES: case-insensitive-filesystem
+
+// RUN: mkdir -p %T/apath
+// RUN: cp %S/Inputs/case-insensitive-include.h %T
+// RUN: cd %T
+// RUN: %clang_cc1 -fsyntax-only -fms-compatibility %s -include %s -I %T -verify
+// RUN: %clang_cc1 -fsyntax-only -fms-compatibility -fdiagnostics-parseable-fixits %s -include %s -I %T 2>&1 | FileCheck %s
+
+#include "..\Output\.\case-insensitive-include.h"
+#include "..\Output\.\Case-Insensitive-Include.h" // expected-warning {{non-portable path}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:50}:"\"..\\Output\\.\\case-insensitive-include.h\""
+#include "..\output\.\case-insensitive-include.h" // expected-warning {{non-portable path}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:50}:"\"..\\Output\\.\\case-insensitive-include.h\""
+
+#include "apath\..\.\case-insensitive-include.h"
+#include "apath\..\.\Case-Insensitive-Include.h" // expected-warning {{non-portable path}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:49}:"\"apath\\..\\.\\case-insensitive-include.h\""
+#include "APath\..\.\case-insensitive-include.h" // For the sake of efficiency, this case is not diagnosed. :-(
Index: test/Lexer/Inputs/case-insensitive-include.h
===
--- /dev/null
+++ test/Lexer/Inputs/case-insensitive-include.h
@@ -0,0 +1,5 @@
+#pragma once
+
+struct S {
+  int x;
+};
Index: lib/Lex/PPDirectives.cpp
===
--- lib/Lex/PPDirectives.cpp
+++ lib/Lex/PPDirectives.cpp
@@ -24,6 +24,9 @@
 #include "clang/Lex/ModuleLoader.h"
 #include "clang/Lex/Pragma.h"
 #include "llvm/ADT/APInt.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 via cfe-commits]

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 aren't shown in some modes of operation; it might be helpful 
> for this diagnostic to be a bit more explicit about what's non-portable since 
> without the fixit it's not obvious. Maybe something like
> 
> "non-portable path to file '%0'; specified path differs in case from file 
> name on disk"
> 
> where %0 is the corrected path?
Good suggestion.


Comment at: lib/Lex/PPDirectives.cpp:1763-1765
@@ +1762,5 @@
+  // than the one we're about to open.
+  //
+  // Because testing for non-portable paths is expensive, only do it if the
+  // warning is not currently ignored.
+  const bool CheckIncludePathPortability =

rsmith wrote:
> Looks like this comment is stale?
Oops, yeah.


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 via cfe-commits]

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
  include/clang/Basic/FileManager.h
  include/clang/Basic/VirtualFileSystem.h
  include/clang/Lex/DirectoryLookup.h
  include/clang/Lex/HeaderSearch.h
  lib/Basic/FileManager.cpp
  lib/Basic/VirtualFileSystem.cpp
  lib/Lex/HeaderSearch.cpp
  lib/Lex/PPDirectives.cpp
  test/Lexer/Inputs/case-insensitive-include.h
  test/Lexer/case-insensitive-include-ms.c
  test/Lexer/case-insensitive-include.c
  test/PCH/case-insensitive-include.c

Index: test/PCH/case-insensitive-include.c
===
--- test/PCH/case-insensitive-include.c
+++ test/PCH/case-insensitive-include.c
@@ -2,7 +2,7 @@
 
 // Test this without pch.
 // RUN: cp %S/Inputs/case-insensitive-include.h %T
-// RUN: %clang_cc1 -fsyntax-only %s -include %s -I %T -verify
+// RUN: %clang_cc1 -Wno-nonportable-include-path -fsyntax-only %s -include %s -I %T -verify
 
 // Test with pch.
 // RUN: %clang_cc1 -emit-pch -o %t.pch %s -I %T
Index: test/Lexer/case-insensitive-include.c
===
--- /dev/null
+++ test/Lexer/case-insensitive-include.c
@@ -0,0 +1,27 @@
+// REQUIRES: case-insensitive-filesystem
+
+// RUN: mkdir -p %T/apath
+// RUN: cp %S/Inputs/case-insensitive-include.h %T
+// RUN: cd %T
+// RUN: %clang_cc1 -fsyntax-only %s -include %s -I %T -verify
+// RUN: %clang_cc1 -fsyntax-only -fdiagnostics-parseable-fixits %s -include %s -I %T 2>&1 | FileCheck %s
+
+#include "case-insensitive-include.h"
+#include "Case-Insensitive-Include.h" // expected-warning {{non-portable path}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:38}:"\"case-insensitive-include.h\""
+
+#include "../Output/./case-insensitive-include.h"
+#include "../Output/./Case-Insensitive-Include.h" // expected-warning {{non-portable path}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:50}:"\"../Output/./case-insensitive-include.h\""
+#include "../output/./case-insensitive-include.h" // expected-warning {{non-portable path}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:50}:"\"../Output/./case-insensitive-include.h\""
+
+#include "apath/.././case-insensitive-include.h"
+#include "apath/.././Case-Insensitive-Include.h" // expected-warning {{non-portable path}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:49}:"\"apath/.././case-insensitive-include.h\""
+#include "APath/.././case-insensitive-include.h" // For the sake of efficiency, this case is not diagnosed. :-(
+
+#include "../Output/./apath/.././case-insensitive-include.h"
+#include "../Output/./APath/.././case-insensitive-include.h" // For the sake of efficiency, this case is not diagnosed. :-(
+#include "../output/./apath/.././case-insensitive-include.h" // expected-warning {{non-portable path}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:61}:"\"../Output/./apath/.././case-insensitive-include.h\""
Index: test/Lexer/case-insensitive-include-ms.c
===
--- /dev/null
+++ test/Lexer/case-insensitive-include-ms.c
@@ -0,0 +1,18 @@
+// REQUIRES: case-insensitive-filesystem
+
+// RUN: mkdir -p %T/apath
+// RUN: cp %S/Inputs/case-insensitive-include.h %T
+// RUN: cd %T
+// RUN: %clang_cc1 -fsyntax-only -fms-compatibility %s -include %s -I %T -verify
+// RUN: %clang_cc1 -fsyntax-only -fms-compatibility -fdiagnostics-parseable-fixits %s -include %s -I %T 2>&1 | FileCheck %s
+
+#include "..\Output\.\case-insensitive-include.h"
+#include "..\Output\.\Case-Insensitive-Include.h" // expected-warning {{non-portable path}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:50}:"\"..\\Output\\.\\case-insensitive-include.h\""
+#include "..\output\.\case-insensitive-include.h" // expected-warning {{non-portable path}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:50}:"\"..\\Output\\.\\case-insensitive-include.h\""
+
+#include "apath\..\.\case-insensitive-include.h"
+#include "apath\..\.\Case-Insensitive-Include.h" // expected-warning {{non-portable path}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:49}:"\"apath\\..\\.\\case-insensitive-include.h\""
+#include "APath\..\.\case-insensitive-include.h" // For the sake of efficiency, this case is not diagnosed. :-(
Index: test/Lexer/Inputs/case-insensitive-include.h
===
--- /dev/null
+++ test/Lexer/Inputs/case-insensitive-include.h
@@ -0,0 +1,5 @@
+#pragma once
+
+struct S {
+  int x;
+};
Index: lib/Lex/PPDirectives.cpp
===
--- lib/Lex/PPDirectives.cpp
+++ lib/Lex/PPDirectives.cpp
@@ -24,6 +24,9 @@
 #include "clang/Lex/ModuleLoader.h"
 #include "clang/Lex/Pragma.h"
 #include 

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 via cfe-commits]

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 on a 
fix.


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 via cfe-commits]

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



___
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 via cfe-commits]

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
  include/clang/Basic/DiagnosticLexKinds.td
  include/clang/Basic/FileManager.h
  include/clang/Basic/VirtualFileSystem.h
  include/clang/Lex/DirectoryLookup.h
  include/clang/Lex/HeaderSearch.h
  lib/Basic/FileManager.cpp
  lib/Basic/VirtualFileSystem.cpp
  lib/Lex/HeaderSearch.cpp
  lib/Lex/PPDirectives.cpp
  test/Lexer/Inputs/case-insensitive-include.h
  test/Lexer/case-insensitive-include.c
  test/PCH/case-insensitive-include.c

Index: test/PCH/case-insensitive-include.c
===
--- test/PCH/case-insensitive-include.c
+++ test/PCH/case-insensitive-include.c
@@ -2,7 +2,7 @@
 
 // Test this without pch.
 // RUN: cp %S/Inputs/case-insensitive-include.h %T
-// RUN: %clang_cc1 -fsyntax-only %s -include %s -I %T -verify
+// RUN: %clang_cc1 -Wno-nonportable-include-path -fsyntax-only %s -include %s -I %T -verify
 
 // Test with pch.
 // RUN: %clang_cc1 -emit-pch -o %t.pch %s -I %T
Index: test/Lexer/case-insensitive-include.c
===
--- /dev/null
+++ test/Lexer/case-insensitive-include.c
@@ -0,0 +1,42 @@
+// REQUIRES: case-insensitive-filesystem
+
+// RUN: mkdir -p %T/apath
+// RUN: cp %S/Inputs/case-insensitive-include.h %T
+// RUN: %clang_cc1 -fsyntax-only %s -include %s -I %T -verify
+
+// RUN: cp %S/Inputs/case-insensitive-include.h %T
+// RUN: %clang_cc1 -fsyntax-only -fms-compatibility -DMS_COMPATIBILITY %s -include %s -I %T -verify
+
+#ifndef HEADER
+#define HEADER
+
+#include "case-insensitive-include.h"
+#include "Case-Insensitive-Include.h" // expected-warning {{Non-portable path}}
+
+#include "../Output/./case-insensitive-include.h"
+#include "../Output/./Case-Insensitive-Include.h" // expected-warning {{Non-portable path}}
+#include "../output/./case-insensitive-include.h" // expected-warning {{Non-portable path}}
+
+#include "apath/.././case-insensitive-include.h"
+#include "apath/.././Case-Insensitive-Include.h" // expected-warning {{Non-portable path}}
+#include "APath/.././case-insensitive-include.h" // For the sake of efficiency, this case is not diagnosed. :-(
+
+#include "../Output/./apath/.././case-insensitive-include.h"
+#include "../Output/./APath/.././case-insensitive-include.h" // For the sake of efficiency, this case is not diagnosed. :-(
+#include "../output/./apath/.././case-insensitive-include.h" // expected-warning {{Non-portable path}}
+
+#if MS_COMPATIBILITY
+#include "..\Output\.\case-insensitive-include.h"
+#include "..\Output\.\Case-Insensitive-Include.h" // expected-warning {{Non-portable path}}
+#include "..\output\.\case-insensitive-include.h" // expected-warning {{Non-portable path}}
+
+#include "apath\..\.\case-insensitive-include.h"
+#include "apath\..\.\Case-Insensitive-Include.h" // expected-warning {{Non-portable path}}
+#include "APath\..\.\case-insensitive-include.h" // For the sake of efficiency, this case is not diagnosed. :-(
+#endif
+
+#else
+
+#include "Case-Insensitive-Include.h" // expected-warning {{Non-portable path}}
+
+#endif
Index: test/Lexer/Inputs/case-insensitive-include.h
===
--- /dev/null
+++ test/Lexer/Inputs/case-insensitive-include.h
@@ -0,0 +1,5 @@
+#pragma once
+
+struct S {
+  int x;
+};
Index: lib/Lex/PPDirectives.cpp
===
--- lib/Lex/PPDirectives.cpp
+++ lib/Lex/PPDirectives.cpp
@@ -24,6 +24,9 @@
 #include "clang/Lex/ModuleLoader.h"
 #include "clang/Lex/Pragma.h"
 #include "llvm/ADT/APInt.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/StringExtras.h"
+#include "llvm/ADT/iterator_range.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/SaveAndRestore.h"
@@ -1497,6 +1500,40 @@
   ("@import " + PathString + ";").str());
 }
 
+namespace
+{
+  // Given a vector of path components and a string containing the real
+  // path to the file, build a properly-cased replacement in the vector,
+  // and return true if the replacement should be suggested.
+  bool TrySimplifyPath(SmallVectorImpl& Components,
+   StringRef RealPathName) {
+auto iRealPathComponent = llvm::sys::path::rbegin(RealPathName);
+auto iRealPathComponentEnd = llvm::sys::path::rend(RealPathName);
+int Cnt = 0;
+bool SuggestReplacement = false;
+for(auto& Component : llvm::reverse(Components)) {
+  if ("." == Component) {
+  } else if (".." == Component) {
+++Cnt;
+  } else if (Cnt) {
+--Cnt;
+  } else if (iRealPathComponent != iRealPathComponentEnd) {
+if (Component != *iRealPathComponent) {
+  // If these 

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 via cfe-commits]

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.


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 via cfe-commits]

eric_niebler added a comment.

"pi"
clangclang 

"ng"


http://reviews.llvm.org/D19843



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Eric Niebler via cfe-commits]

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 accepted, I will need to decide what to do about the test, which 
is likely to be somewhat flakey due to the very nature of the problem.

http://reviews.llvm.org/D19843

Files:
  include/clang/Basic/DiagnosticGroups.td
  include/clang/Basic/DiagnosticLexKinds.td
  include/clang/Basic/FileManager.h
  include/clang/Basic/VirtualFileSystem.h
  include/clang/Lex/DirectoryLookup.h
  include/clang/Lex/HeaderSearch.h
  lib/Basic/FileManager.cpp
  lib/Basic/VirtualFileSystem.cpp
  lib/Lex/HeaderSearch.cpp
  lib/Lex/PPDirectives.cpp
  test/Lexer/Inputs/case-insensitive-include.h
  test/Lexer/case-insensitive-include.c
  test/PCH/case-insensitive-include.c

Index: test/PCH/case-insensitive-include.c
===
--- test/PCH/case-insensitive-include.c
+++ test/PCH/case-insensitive-include.c
@@ -2,7 +2,7 @@
 
 // Test this without pch.
 // RUN: cp %S/Inputs/case-insensitive-include.h %T
-// RUN: %clang_cc1 -fsyntax-only %s -include %s -I %T -verify
+// RUN: %clang_cc1 -Wno-nonportable-include-path -fsyntax-only %s -include %s -I %T -verify
 
 // Test with pch.
 // RUN: %clang_cc1 -emit-pch -o %t.pch %s -I %T
Index: test/Lexer/case-insensitive-include.c
===
--- /dev/null
+++ test/Lexer/case-insensitive-include.c
@@ -0,0 +1,42 @@
+// REQUIRES: case-insensitive-filesystem
+
+// RUN: mkdir -p %T/apath
+// RUN: cp %S/Inputs/case-insensitive-include.h %T
+// RUN: %clang_cc1 -fsyntax-only %s -include %s -I %T -verify
+
+// RUN: cp %S/Inputs/case-insensitive-include.h %T
+// RUN: %clang_cc1 -fsyntax-only -fms-compatibility -DMS_COMPATIBILITY %s -include %s -I %T -verify
+
+#ifndef HEADER
+#define HEADER
+
+#include "case-insensitive-include.h"
+#include "Case-Insensitive-Include.h" // expected-warning {{Non-portable path}}
+
+#include "../Output/./case-insensitive-include.h"
+#include "../Output/./Case-Insensitive-Include.h" // expected-warning {{Non-portable path}}
+#include "../output/./case-insensitive-include.h" // expected-warning {{Non-portable path}}
+
+#include "apath/.././case-insensitive-include.h"
+#include "apath/.././Case-Insensitive-Include.h" // expected-warning {{Non-portable path}}
+#include "APath/.././case-insensitive-include.h" // For the sake of efficiency, this case is not diagnosed. :-(
+
+#include "../Output/./apath/.././case-insensitive-include.h"
+#include "../Output/./APath/.././case-insensitive-include.h" // For the sake of efficiency, this case is not diagnosed. :-(
+#include "../output/./apath/.././case-insensitive-include.h" // expected-warning {{Non-portable path}}
+
+#if MS_COMPATIBILITY
+#include "..\Output\.\case-insensitive-include.h"
+#include "..\Output\.\Case-Insensitive-Include.h" // expected-warning {{Non-portable path}}
+#include "..\output\.\case-insensitive-include.h" // expected-warning {{Non-portable path}}
+
+#include "apath\..\.\case-insensitive-include.h"
+#include "apath\..\.\Case-Insensitive-Include.h" // expected-warning {{Non-portable path}}
+#include "APath\..\.\case-insensitive-include.h" // For the sake of efficiency, this case is not diagnosed. :-(
+#endif
+
+#else
+
+#include "Case-Insensitive-Include.h" // expected-warning {{Non-portable path}}
+
+#endif
Index: test/Lexer/Inputs/case-insensitive-include.h
===
--- /dev/null
+++ test/Lexer/Inputs/case-insensitive-include.h
@@ -0,0 +1,5 @@
+#pragma once
+
+struct S {
+  int x;
+};
Index: lib/Lex/PPDirectives.cpp
===
--- lib/Lex/PPDirectives.cpp
+++ lib/Lex/PPDirectives.cpp
@@ -24,6 +24,9 @@
 #include "clang/Lex/ModuleLoader.h"
 #include "clang/Lex/Pragma.h"
 #include "llvm/ADT/APInt.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/StringExtras.h"
+#include "llvm/ADT/iterator_range.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/SaveAndRestore.h"
@@ -1661,6 +1664,53 @@
 }
   }
 
+  // Issue a diagnostic if the name of the file on disk has a different case
+  // than the one we're about to open.
+
+  // Because typo correction is expensive, only do it if the implicit
+  // function declaration is going to be treated as an error.
+  if (File && !File->tryGetRealPathName().empty() &&
+!Diags->isIgnored(diag::pp_nonportable_path, FilenameTok.getLocation())) {
+StringRef Name = LangOpts.MSVCCompat ? NormalizedPath.c_str() : Filename;
+StringRef RealPathName = File->tryGetRealPathName();
+SmallVector Components(llvm::sys::path::begin(Name),
+