Re: [PATCH] D79595: Fix bugs when an included file name is typo corrected.

[Hans Wennborg via cfe-commits]

+thakis who I think fell off the cc list

On Thu, May 14, 2020 at 12:51 AM Abhinav Gaba via Phabricator
 wrote:
>
> abhinavgaba added inline comments.
>
>
> 
> Comment at: clang/test/Lexer/case-insensitive-include-win.c:8
>  // RUN: touch %t.dir/foo.h
> -// RUN: not %clang_cl /FI\\?\%t.dir\FOO.h /WX -Xclang -verify -fsyntax-only 
> %s 2>&1 | FileCheck %s
> +// RUN: not %clang_cl /FI\\?\%t.dir\FOO.h /WX -fsyntax-only %s 2>&1 | 
> FileCheck %s
>
> 
> abhinavgaba wrote:
> > The test is currently failing if path of an existing directory in a network 
> > mounted drive, using forward slashes, is in CPATH. (e.g. `export 
> > CPATH=//abc.def.com/dir1`).
> >
> > The error in that case is:
> > ```
> > fatal error: cannot open file
> >   
> > '//abc.def.com/dir1\?\\test\Lexer\Output\case-insensitive-include-win.c.tmp.dir\FOO.h':
> >   The specified path is invalid.
> > ```
> >
> Did you get a chance to look at this?
>
>
> Repository:
>   rG LLVM Github Monorepo
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D79595/new/
>
> https://reviews.llvm.org/D79595
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79595: Fix bugs when an included file name is typo corrected.

[Abhinav Gaba via Phabricator via cfe-commits]

abhinavgaba added inline comments.



Comment at: clang/test/Lexer/case-insensitive-include-win.c:8
 // RUN: touch %t.dir/foo.h
-// RUN: not %clang_cl /FI\\?\%t.dir\FOO.h /WX -Xclang -verify -fsyntax-only %s 
2>&1 | FileCheck %s
+// RUN: not %clang_cl /FI\\?\%t.dir\FOO.h /WX -fsyntax-only %s 2>&1 | 
FileCheck %s
 

abhinavgaba wrote:
> The test is currently failing if path of an existing directory in a network 
> mounted drive, using forward slashes, is in CPATH. (e.g. `export 
> CPATH=//abc.def.com/dir1`).
> 
> The error in that case is:
> ```
> fatal error: cannot open file
>   
> '//abc.def.com/dir1\?\\test\Lexer\Output\case-insensitive-include-win.c.tmp.dir\FOO.h':
>   The specified path is invalid.
> ```
> 
Did you get a chance to look at this?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79595/new/

https://reviews.llvm.org/D79595



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

[PATCH] D79595: Fix bugs when an included file name is typo corrected.

[Abhinav Gaba via Phabricator via cfe-commits]

abhinavgaba added inline comments.



Comment at: clang/test/Lexer/case-insensitive-include-win.c:8
 // RUN: touch %t.dir/foo.h
-// RUN: not %clang_cl /FI\\?\%t.dir\FOO.h /WX -Xclang -verify -fsyntax-only %s 
2>&1 | FileCheck %s
+// RUN: not %clang_cl /FI\\?\%t.dir\FOO.h /WX -fsyntax-only %s 2>&1 | 
FileCheck %s
 

The test is currently failing if path of an existing directory in a network 
mounted drive, using forward slashes, is in CPATH. (e.g. `export 
CPATH=//abc.def.com/dir1`).

The error in that case is:
```
fatal error: cannot open file
  
'//abc.def.com/dir1\?\\test\Lexer\Output\case-insensitive-include-win.c.tmp.dir\FOO.h':
  The specified path is invalid.
```



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79595/new/

https://reviews.llvm.org/D79595



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

[PATCH] D79595: Fix bugs when an included file name is typo corrected.

[Nico Weber via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG51d938bc9443: Fix bugs when an included file name is typo 
corrected. (authored by thakis).
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79595/new/

https://reviews.llvm.org/D79595

Files:
  clang/include/clang/Lex/Preprocessor.h
  clang/lib/Lex/PPDirectives.cpp
  clang/test/Lexer/case-insensitive-include-pr31836.sh
  clang/test/Lexer/case-insensitive-include-win.c

Index: clang/test/Lexer/case-insensitive-include-win.c
===
--- clang/test/Lexer/case-insensitive-include-win.c
+++ clang/test/Lexer/case-insensitive-include-win.c
@@ -5,6 +5,6 @@
 // REQUIRES: system-windows
 // RUN: mkdir -p %t.dir
 // RUN: touch %t.dir/foo.h
-// RUN: not %clang_cl /FI\\?\%t.dir\FOO.h /WX -Xclang -verify -fsyntax-only %s 2>&1 | FileCheck %s
+// RUN: not %clang_cl /FI\\?\%t.dir\FOO.h /WX -fsyntax-only %s 2>&1 | FileCheck %s
 
 // CHECK: non-portable path to file '"\\?\
Index: clang/test/Lexer/case-insensitive-include-pr31836.sh
===
--- clang/test/Lexer/case-insensitive-include-pr31836.sh
+++ clang/test/Lexer/case-insensitive-include-pr31836.sh
@@ -2,7 +2,8 @@
 
 // RUN: mkdir -p %t
 // RUN: touch %t/case-insensitive-include-pr31836.h
-// RUN: echo "#include \"%t/Case-Insensitive-Include-Pr31836.h\"" | %clang_cc1 -E - 2>&1 | FileCheck %s
+// RUN: echo "#include \"?%t/Case-Insensitive-Include-Pr31836.h\"" | not %clang_cc1 -E - 2>&1 | FileCheck %s
 
+// CHECK: error: {{.*}}file not found, did you mean
 // CHECK: warning: non-portable path to file
 // CHECK-SAME: /case-insensitive-include-pr31836.h
Index: clang/lib/Lex/PPDirectives.cpp
===
--- clang/lib/Lex/PPDirectives.cpp
+++ clang/lib/Lex/PPDirectives.cpp
@@ -1716,11 +1716,11 @@
 }
 
 Optional Preprocessor::LookupHeaderIncludeOrImport(
-const DirectoryLookup *, StringRef Filename,
+const DirectoryLookup *, StringRef& Filename,
 SourceLocation FilenameLoc, CharSourceRange FilenameRange,
 const Token , bool , bool IsImportDecl,
 bool , const DirectoryLookup *LookupFrom,
-const FileEntry *LookupFromFile, StringRef LookupFilename,
+const FileEntry *LookupFromFile, StringRef& LookupFilename,
 SmallVectorImpl , SmallVectorImpl ,
 ModuleMap::KnownHeader , bool isAngled) {
   Optional File = LookupFile(
@@ -1789,21 +1789,10 @@
   return Filename;
 };
 StringRef TypoCorrectionName = CorrectTypoFilename(Filename);
-
-#ifndef _WIN32
-// Normalize slashes when compiling with -fms-extensions on non-Windows.
-// This is unnecessary on Windows since the filesystem there handles
-// backslashes.
-SmallString<128> NormalizedTypoCorrectionPath;
-if (LangOpts.MicrosoftExt) {
-  NormalizedTypoCorrectionPath = TypoCorrectionName;
-  llvm::sys::path::native(NormalizedTypoCorrectionPath);
-  TypoCorrectionName = NormalizedTypoCorrectionPath;
-}
-#endif
+StringRef TypoCorrectionLookupName = CorrectTypoFilename(LookupFilename);
 
 Optional File = LookupFile(
-FilenameLoc, TypoCorrectionName, isAngled, LookupFrom, LookupFromFile,
+FilenameLoc, TypoCorrectionLookupName, isAngled, LookupFrom, LookupFromFile,
 CurDir, Callbacks ?  : nullptr,
 Callbacks ?  : nullptr, , ,
 /*IsFrameworkFound=*/nullptr);
@@ -1818,6 +1807,7 @@
   // We found the file, so set the Filename to the name after typo
   // correction.
   Filename = TypoCorrectionName;
+  LookupFilename = TypoCorrectionLookupName;
   return File;
 }
   }
@@ -2074,8 +2064,6 @@
   if (Callbacks && !IsImportDecl) {
 // Notify the callback object that we've seen an inclusion directive.
 // FIXME: Use a different callback for a pp-import?
-// FIXME: Passes wrong filename if LookupHeaderIncludeOrImport() did
-// typo correction.
 Callbacks->InclusionDirective(
 HashLoc, IncludeTok, LookupFilename, isAngled, FilenameRange,
 File ? >getFileEntry() : nullptr, SearchPath, RelativePath,
@@ -2102,7 +2090,6 @@
   !IsMapped && !File->getFileEntry().tryGetRealPathName().empty();
 
   if (CheckIncludePathPortability) {
-// FIXME: Looks at the wrong filename if we did typo correction.
 StringRef Name = LookupFilename;
 StringRef NameWithoriginalSlashes = Filename;
 #if defined(_WIN32)
Index: clang/include/clang/Lex/Preprocessor.h
===
--- clang/include/clang/Lex/Preprocessor.h
+++ clang/include/clang/Lex/Preprocessor.h
@@ -2242,11 +2242,11 @@
   };
 
   Optional LookupHeaderIncludeOrImport(
-  const DirectoryLookup *, StringRef Filename,
+  const DirectoryLookup *, StringRef ,
   SourceLocation FilenameLoc, 

[PATCH] D79595: Fix bugs when an included file name is typo corrected.

[Hans Wennborg via Phabricator via cfe-commits]

hans accepted this revision.
hans added a comment.
This revision is now accepted and ready to land.

lgtm


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79595/new/

https://reviews.llvm.org/D79595



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

[PATCH] D79595: Fix bugs when an included file name is typo corrected.

[Nico Weber via Phabricator via cfe-commits]

thakis created this revision.
thakis added a reviewer: hans.

D52774  fixed a bug with typo correction of 
includes, but didn't add
a test.

D65907  then broke recovery of typo correction 
of includes again,
because it extracted the code that writes to Filename to a separate
function that took the parameter not by reference.

Fix that, and also don't repeat the slash normalization computation
and fix both lookup and regular file name after recovery.


https://reviews.llvm.org/D79595

Files:
  clang/include/clang/Lex/Preprocessor.h
  clang/lib/Lex/PPDirectives.cpp
  clang/test/Lexer/case-insensitive-include-pr31836.sh
  clang/test/Lexer/case-insensitive-include-win.c

Index: clang/test/Lexer/case-insensitive-include-win.c
===
--- clang/test/Lexer/case-insensitive-include-win.c
+++ clang/test/Lexer/case-insensitive-include-win.c
@@ -5,6 +5,6 @@
 // REQUIRES: system-windows
 // RUN: mkdir -p %t.dir
 // RUN: touch %t.dir/foo.h
-// RUN: not %clang_cl /FI\\?\%t.dir\FOO.h /WX -Xclang -verify -fsyntax-only %s 2>&1 | FileCheck %s
+// RUN: not %clang_cl /FI\\?\%t.dir\FOO.h /WX -fsyntax-only %s 2>&1 | FileCheck %s
 
 // CHECK: non-portable path to file '"\\?\
Index: clang/test/Lexer/case-insensitive-include-pr31836.sh
===
--- clang/test/Lexer/case-insensitive-include-pr31836.sh
+++ clang/test/Lexer/case-insensitive-include-pr31836.sh
@@ -2,7 +2,8 @@
 
 // RUN: mkdir -p %t
 // RUN: touch %t/case-insensitive-include-pr31836.h
-// RUN: echo "#include \"%t/Case-Insensitive-Include-Pr31836.h\"" | %clang_cc1 -E - 2>&1 | FileCheck %s
+// RUN: echo "#include \"?%t/Case-Insensitive-Include-Pr31836.h\"" | not %clang_cc1 -E - 2>&1 | FileCheck %s
 
+// CHECK: error: {{.*}}file not found, did you mean
 // CHECK: warning: non-portable path to file
 // CHECK-SAME: /case-insensitive-include-pr31836.h
Index: clang/lib/Lex/PPDirectives.cpp
===
--- clang/lib/Lex/PPDirectives.cpp
+++ clang/lib/Lex/PPDirectives.cpp
@@ -1716,11 +1716,11 @@
 }
 
 Optional Preprocessor::LookupHeaderIncludeOrImport(
-const DirectoryLookup *, StringRef Filename,
+const DirectoryLookup *, StringRef& Filename,
 SourceLocation FilenameLoc, CharSourceRange FilenameRange,
 const Token , bool , bool IsImportDecl,
 bool , const DirectoryLookup *LookupFrom,
-const FileEntry *LookupFromFile, StringRef LookupFilename,
+const FileEntry *LookupFromFile, StringRef& LookupFilename,
 SmallVectorImpl , SmallVectorImpl ,
 ModuleMap::KnownHeader , bool isAngled) {
   Optional File = LookupFile(
@@ -1789,21 +1789,10 @@
   return Filename;
 };
 StringRef TypoCorrectionName = CorrectTypoFilename(Filename);
-
-#ifndef _WIN32
-// Normalize slashes when compiling with -fms-extensions on non-Windows.
-// This is unnecessary on Windows since the filesystem there handles
-// backslashes.
-SmallString<128> NormalizedTypoCorrectionPath;
-if (LangOpts.MicrosoftExt) {
-  NormalizedTypoCorrectionPath = TypoCorrectionName;
-  llvm::sys::path::native(NormalizedTypoCorrectionPath);
-  TypoCorrectionName = NormalizedTypoCorrectionPath;
-}
-#endif
+StringRef TypoCorrectionLookupName = CorrectTypoFilename(LookupFilename);
 
 Optional File = LookupFile(
-FilenameLoc, TypoCorrectionName, isAngled, LookupFrom, LookupFromFile,
+FilenameLoc, TypoCorrectionLookupName, isAngled, LookupFrom, LookupFromFile,
 CurDir, Callbacks ?  : nullptr,
 Callbacks ?  : nullptr, , ,
 /*IsFrameworkFound=*/nullptr);
@@ -1818,6 +1807,7 @@
   // We found the file, so set the Filename to the name after typo
   // correction.
   Filename = TypoCorrectionName;
+  LookupFilename = TypoCorrectionLookupName;
   return File;
 }
   }
@@ -2074,8 +2064,6 @@
   if (Callbacks && !IsImportDecl) {
 // Notify the callback object that we've seen an inclusion directive.
 // FIXME: Use a different callback for a pp-import?
-// FIXME: Passes wrong filename if LookupHeaderIncludeOrImport() did
-// typo correction.
 Callbacks->InclusionDirective(
 HashLoc, IncludeTok, LookupFilename, isAngled, FilenameRange,
 File ? >getFileEntry() : nullptr, SearchPath, RelativePath,
@@ -2102,7 +2090,6 @@
   !IsMapped && !File->getFileEntry().tryGetRealPathName().empty();
 
   if (CheckIncludePathPortability) {
-// FIXME: Looks at the wrong filename if we did typo correction.
 StringRef Name = LookupFilename;
 StringRef NameWithoriginalSlashes = Filename;
 #if defined(_WIN32)
Index: clang/include/clang/Lex/Preprocessor.h
===
--- clang/include/clang/Lex/Preprocessor.h
+++ clang/include/clang/Lex/Preprocessor.h
@@ -2242,11