search

HaveF / include-what-you-use

Automatically exported from code.google.com/p/include-what-you-use
Other
0 stars 0 forks source link

IWYU case sensitive on windows #130

Open GoogleCodeExporter opened 9 years ago

GoogleCodeExporter commented 9 years ago 

Hi. When I run IWYU on a cpp file normally I would get the report for that cpp 
file and also its .h header. However, I found some strange case where this was 
not happening. I am runing the trunk version on a Windows system.

I debugged the issue, and it all came down to a problem of case sensitivity. As 
an example, we have this:

Foo.cpp

#include "foo.h"

Foo::Foo()
{
}

Foo::~Foo()
{
}

Foo.h

#ifndef _FOO_H_
#define _FOO_H_

class Foo
{
public:

Foo() {}
~Foo() {}

private
}

#endif _FOO_H_

When IWYU reaches IwyuPreprocessorInfo::FileChanged_EnterFile it will check if 
the file should be added to the report by calling BelongsToMainCompilationUnit. 
This function is doing this inside:

if (GetCanonicalName(GetFilePath(includee)) ==
      GetCanonicalName(GetFilePath(main_file_)))

which returns false, because the comparison is foo == Foo. We get foo instead 
of Foo, because the file is included in the cpp as

#include "foo.h"

On a case insensitive system this should not happen don't you think?

Original issue reported on code.google.com by dpun...@gmail.com on 5 May 2014 at 7:56

GoogleCodeExporter commented 9 years ago 

If it's any help, I fixed all my issues related to this by adding this at the 
end of iwyu_path_util.cc : GetCanonicalName(string file_path)

#ifdef _WIN32
  std::transform(file_path.begin(), file_path.end(), file_path.begin(), ::tolower);
#endif

What do you think?

Original comment by dpun...@gmail.com on 6 May 2014 at 6:01

GoogleCodeExporter commented 9 years ago 
I think that's the best we can do, I went Googling for detecting the 
case-sensitivity of file systems yesterday and found nothing.

The alternative is to expose a flag just for this, but it seems a little 
pessimistic. We could expose an override on the command-line for people using 
case-insensitive Macs or case-sensitive Windows filesystems (I *think* both are 
possible), so they can force their preference if we get the heuristic defaults 
wrong.

Original comment by kim.gras...@gmail.com on 6 May 2014 at 6:23

GoogleCodeExporter commented 9 years ago 

The later sounds good.
Thank you Kim

Original comment by dpun...@gmail.com on 6 May 2014 at 7:52

GoogleCodeExporter commented 9 years ago 
Small correction: Mac OS X by default offers to create case-insensitive file 
system.

I am not in favor of this change.  I don't have good evidence to support my 
opinion, I have only gut feeling and limited personal experience.  So it is 
highly possible, that I am wrong.

Making IWYU completely case-aware is fairly hard (file names are used in a lot 
of places).  It doesn't help that there is no easy way to detect if file system 
is case-sensitive.  The fix seems
  incomplete - case-insensitive path comparison is done only in one place,
  and unreliable - we are just guessing if file system is case-insensitive.

It's not that the fix is bad, I greatly appreciate your efforts, dpunset.  I 
think that the task at hand is just more involved.

I prefer to perform such a fix only when it's usefult for a noticeable number 
of users.  dpunset, is it a minimal repro case created from real-world code or 
is it an artificial test case?

Original comment by vsap...@gmail.com on 6 May 2014 at 10:53

GoogleCodeExporter commented 9 years ago 

Hi!

No it is not a minimal repro case, this appeared evaluating the tool on a big 
code base. Personally I don't think it's very rare to arrive to this situation.

I agree with your statements about being a partial solution, in fact I was 
surprised that just adding those lines fixed the 2 problems we encountered 
because of this. My modification was just to let you know, (like I said) in 
case it was of any help.

What I agree with is Kim's suggestion about adding some option to enforce one 
case or the other; but of course, yes, I would expect a more robust 
implementation to support case-insensitiviness. While I did that fix I saw 
there is another implementation of GetCanonicalName in some other file and I 
wasn't sure if I had to modify it too for example. I just went for the quick 
change to see if it did anything and I was lucky enough that it did.

Original comment by dpun...@gmail.com on 7 May 2014 at 7:52

GoogleCodeExporter commented 9 years ago 
dpunset, is case mismatch accidental or intentional?

In my opinion, IWYU should be case-aware.  But case mismatch is easy to fix in 
source code and it's better not to have case mismatches in the long run (it's 
only my personal opinion, take it with a grain of salt).  That's why I think 
the issue should be fixed, but the priority isn't very high.

Original comment by vsap...@gmail.com on 11 May 2014 at 4:30

GoogleCodeExporter commented 9 years ago 

Accidental yes, but an accident that's bound to happen very easily on a 
case-insensitive system with no harm on the compilation, right?

It's just that for someone who is working on Windows, something like this to 
happen seems like a bug on IWYU. Also it's hard to track if you don't know, 
since the tool don't give any warning or anything.

Original comment by dpun...@gmail.com on 12 May 2014 at 12:08

GoogleCodeExporter commented 9 years ago 
I agree with vsapsai that there are only good things to come from fixing case 
inconsistencies in a code base.

But I also agree with dpunset that IWYU becomes very difficult to use on 
MSVC-only code bases, because parts of IWYU just stop working.

I would prefer it if IWYU could support case insensitive #includes, but it's 
not a priority for me at this time.

Thinking of solution space, I've never seen a case-sensitive Windows system, so 
I think it's a reasonable assumption that _WIN32 -> case-insensitive, as long 
as there's a switch to override the defaults.

Original comment by kim.gras...@gmail.com on 12 May 2014 at 6:29