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

[Bruno Cardoso Lopes via cfe-commits]

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
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.

[Taewook Oh via cfe-commits]

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 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.

[Taewook Oh via cfe-commits]

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 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.

[Taewook Oh via cfe-commits]

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
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.

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.

[Taewook Oh via cfe-commits]

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 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.

[Richard Smith via cfe-commits]

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
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.

[Bruno Cardoso Lopes via cfe-commits]

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) characters in the 
C++ library headers section above. Also make sure to clean this up.


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.

[Richard Smith via cfe-commits]

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 
> 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 =)]


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

Comment doesn't match code: the ASCII range ends at 0x7F.


Comment at: lib/Lex/PPDirectives.cpp:225-229
@@ +224,7 @@
+  Ch += 'a' - 'A';
+#ifdef LLVM_ON_WIN32
+// Normalize path separators for comparison purposes.
+else if (Ch == '\\')
+  Ch = '/';
+#endif
+  }

Rather than hardcoding this platform-specific test here, maybe use 
`llvm::sys::path::is_separator(Ch)`.


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

[Richard Smith via cfe-commits]

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.


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.

[Bruno Cardoso Lopes via cfe-commits]

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 : LowerInclude) {
+// In the ASCII range?

-> "char "


Comment at: lib/Lex/PPDirectives.cpp:1666
@@ +1665,3 @@
+// not 100% correct in the presence of symlinks.
+for(auto  : llvm::reverse(Components)) {
+  if ("." == Component) {

-> "for (auto..."


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

[Taewook Oh via cfe-commits]

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.

[Taewook Oh via cfe-commits]

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
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 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.

[Richard Smith via cfe-commits]

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<
+  "non-portable path '%0' found in preprocessor directive">,
+  InGroup;

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?


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 =

Looks like this comment is stale?


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.

[Richard Smith via cfe-commits]

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 filename.

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

I've left a handful of mostly-stylistic comments inline.



Comment at: include/clang/Basic/DiagnosticLexKinds.td:278
@@ +277,3 @@
+def pp_nonportable_path : Warning<
+  "Non-portable path '%0' found in preprocessor directive.">,
+  InGroup;

Diagnostics should start with a lowercase letter and not end in a period.


Comment at: lib/Lex/PPDirectives.cpp:1503-1504
@@ -1499,1 +1502,4 @@
 
+namespace
+{
+  // Given a vector of path components and a string containing the real

`{` on previous line, please.


Comment at: lib/Lex/PPDirectives.cpp:1508
@@ +1507,3 @@
+  // and return true if the replacement should be suggested.
+  bool TrySimplifyPath(SmallVectorImpl& Components,
+   StringRef RealPathName) {

`&` on the right.


Comment at: lib/Lex/PPDirectives.cpp:1510-1511
@@ +1509,4 @@
+   StringRef RealPathName) {
+auto iRealPathComponent = llvm::sys::path::rbegin(RealPathName);
+auto iRealPathComponentEnd = llvm::sys::path::rend(RealPathName);
+int Cnt = 0;

Local variable names should start with an uppercase letter.


Comment at: lib/Lex/PPDirectives.cpp:1514
@@ +1513,3 @@
+bool SuggestReplacement = false;
+for(auto& Component : llvm::reverse(Components)) {
+  if ("." == Component) {

`&` on the right.


Comment at: lib/Lex/PPDirectives.cpp:1516-1519
@@ +1515,6 @@
+  if ("." == Component) {
+  } else if (".." == Component) {
+++Cnt;
+  } else if (Cnt) {
+--Cnt;
+  } else if (iRealPathComponent != iRealPathComponentEnd) {

Hmm. A `..` doesn't necessarily remove the prior path component, especially in 
the presence of directory symlinks. I suspect it's not worth the trouble of 
trying to get this right, but a comment saying that this is a best-effort 
attempt to handle `..`s would be useful.


Comment at: lib/Lex/PPDirectives.cpp:1704-1705
@@ +1703,4 @@
+  //
+  // Because testing for non-portable paths is expensive, only do it if the
+  // warning is not currently ignored.
+  const bool CheckIncludePathPortability =

Checking whether a warning is enabled can be surprisingly expensive; 
`TrySimplifyPath` might actually be cheaper.


Comment at: test/Lexer/case-insensitive-include.c:4
@@ +3,3 @@
+// 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

Please explicitly `cd` to somewhere around `%T` here rather than assuming you 
know where it is relative to the current directory; we do not guarantee which 
directory the test will be run from and it does differ in practice between 
different ways of running the tests.


Comment at: test/Lexer/case-insensitive-include.c:8-11
@@ +7,6 @@
+// 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
+

What's the reason for this test including this file twice?


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.

[Marek Kurdej via cfe-commits]

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
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),
+