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<NonportableIncludePath>;
----------------
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<StringRef>& 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
