ConvertToQuotedInclude needs to handle leading backslashes (for Win32)

[GoogleCodeExporter]

ConvertToQuotedInclude tidies any stray leading directory separator after is 
has stripped an include path:

      StripLeft(&path, "/");

to work on windows, it needs to do the following:

      StripLeft(&path, "/") || StripLeft(&path, "\\");

Without this a number of the tests are failing like so:

Unexpected summary diffs for tests/virtual_tpl_method.cc:
+++
@@ -1,8 +1,8 @@
 tests/virtual_tpl_method.cc should add these lines:
-#include "tests/indirect.h"
+#include "\tests/indirect.h"

 tests/virtual_tpl_method.cc should remove these lines:
 - #include "tests/direct.h"  // lines XX-XX

 The full include-list for tests/virtual_tpl_method.cc:
-#include "tests/indirect.h"  // for IndirectClass
+#include "\tests/indirect.h"  // for IndirectClass

---

I'm assuming this will no-op on Linux, but I could use an #ifdef _MSC_VER guard 
to ensure this additional strip is only performed on Win32.

Original issue reported on code.google.com by paul.hol...@gmail.com on 16 Jul 2011 at 5:58

Attachments:

• fix_convert_to_quoted_include.patch

[GoogleCodeExporter]

A similar fix is needed to NormalizePath to fix tests/re_fwd_decl.cc on Windows:

(.\tests/re_fwd_decl.h has correct #includes/fwd-decls)
tests/re_fwd_decl.cc:20:1: warning: Indirect needs a declaration, but does not 
provide or directly #include one.

tests/re_fwd_decl.cc should add these lines:
class Indirect;

tests/re_fwd_decl.cc should remove these lines:

The full include-list for tests/re_fwd_decl.cc:
#include "tests/re_fwd_decl.h"
#include "tests/re_fwd_decl-d1.h"       // for Direct (ptr only), FullUse
class Indirect;
---

The fix is to strip any leading '.\':

inline string NormalizeFilePath(const string& path) {
  string result = path;
  while (StripLeft(&result, "./")) {
  }
  while (StripLeft(&result, ".\\")) {
  }
  return result;
}

Again, I'm assuming this will no-op on Linux, but I can add a _MSC_VER guard if 
desired.

Original comment by paul.hol...@gmail.com on 17 Jul 2011 at 10:34

Attachments:

• issue_54_fix_normalize_file_path.patch

[GoogleCodeExporter]

I don't think we should be doing \ -> / conversion anywhere but in 
iwyu_path_util.h.  We already have CanonicalizeFilePath().  I think the right 
solution is to make sure we call CanonicalizeFilePath() every time a file path 
comes into iwyu (either we read it from an #include line, or we get it from 
argv.  We may need to have a reversing routine that converts / to \ when 
emitting a filename -- I don't know enough about windows to say.

I would prefer that fix to something that checks for both / and \ in various 
places.

Original comment by csilv...@gmail.com on 18 Jul 2011 at 11:59

[GoogleCodeExporter]

Ah, yes, I agree it's much better to perform the canonicalisation as paths are 
provided to iwyu. The attached patch achieves the same end result as the 
previous two patches.

Note that I now call CanonicaliseFilePath() from within NormalizeFilePath() - 
the alternatve was to call NormalizeFilePath(CanonicaliseFilePath()) in a 
number of places.

Also, I call CanonicaliseFilePath() before inserting entries into 
search_path_map. The alternative was to do this in 
NormalizeHeaderSearchPaths(), but this would have incorrectly failed to 
deduplicate "c:\blah\foo" and "c:/blah/foo". 

I have a feeling that we won't need to do the reverse transform for Windows 
when emitting filenames - MSVC will happily handle both '#include "foo\bar.h"' 
and '#include "foo/bar.h"', and the latter is more portable. If it turns out 
that this reverse mapping is needed I can look into it at a later date.

Original comment by paul.hol...@gmail.com on 19 Jul 2011 at 9:28

Attachments:

• canonicalise_paths.patch

[GoogleCodeExporter]

Looks good to me.  Just one style nit:

} string path = CanonicalizeFilePath(entry->getName());

Make this a 'const string'.  Then, feel free to commit!

} I have a feeling that we won't need to do the reverse transform for Windows 
when
} emitting filenames

The issue is what do we do if the user already has #include "foo\bar.h".  Will 
we suggest that they remove "foo\bar.h" and add "foo/bar.h"?  But we can cross 
that bridge when we get to it.

Original comment by csilv...@gmail.com on 19 Jul 2011 at 11:07

• Changed state: Started

[GoogleCodeExporter]

Great! I've applied that (with the 'const string' tweak) in r285.

I'll have a quick look at the potential issue that you mention, and raise a 
separate issue if it turns out to be a problem. Speaking for myself, it would 
be no great shame - our coding convention is to always use forward slashes in 
#include directives anyway (for portability), and so it's actually preferable 
for IWYU to flag this up.

Original comment by paul.hol...@gmail.com on 21 Jul 2011 at 7:59

• Changed state: Fixed