Add new options for formatting relative include paths

[dakotahawkins]

NOTE: REVIEW ONLY PR: I still need to add tests for this

Setting PathFormat=PathMode.Absolute_FromParentDirWithFile in combination
with setting FromParentDirWithFile to a filename that exists "toward" the
root of your source directory tree allows us to make include paths
relative to the nearest parent folder containing the specified file.

This is something we do with our projects in a global property sheet.
MSBuild has the awesome built-in property function
"GetDirectoryNameOfFileAbove", which lets you use a known file (e.g. we
have an empty "build.root" in the root of our source tree) to do the same
thing as this feature. Using this functionality lets you specify things
like additional include/link dirs in a common way that works everywhere so
you don't have to spam a lot of relative paths in your project settings or
include directives.

In other words, with that set, any file can include any header by
specifying its path relative to the root of our source.

To "correctly" handle system includes and includes that are in the same
directory as the file you're working on (that we want to leave path-less),
IgnoreFileRelative was changed from a boolean setting to an enum, where
Never/Always should be the same as false/true and
InSameDirectory/InSameOrSubDirectory provide some more control over which
directives to skip.

I'm not entirely happy with the IgnoreFileRelative setting, I don't think
it's very clear what it's supposed to do (in fact, I don't think I knew
what it did before working on this), so perhaps this could be improved
somehow in the future.

[dakotahawkins]

This functionality is somewhat difficult to describe...

[Wumpf]

Yeah I never heard of this whole thing and it took me a while indeed. I think your description here is quite good after all though.

Suggestion for a slight improvement:

[DisplayName("Filename for Absolute_FromParentDirWithFile")]
[Description("The Absolute_FromParentDirWithFile mode will look for this file in all parent directories and make include paths absolute from its location if found.")]

[dakotahawkins]

Was this just left over from an old version? VS2017 complains about it.

[Wumpf]

Hmm odd. With the newest VS2017 version I'm hitting this as well. Sounds like I should make sure stuff works still in VS2015 before I upload to the marketplace.
Better remove it for good

[dakotahawkins]

This is similar to the previous functionality, but should be more reliable since each part of the path is normalized with GetFileSystemInfos. It's important to get this right, because eventually we rely on string comparisons to determine whether a file is in/under a directory path.

[Wumpf]

Interesting. I'm not sure I could really follow the discussion on that SO page. Can you give me an example where GetExactPathName gives a different result than GetFileSystemCasing for absolute paths?
Would be really nice if we could use the same function for both relative and absolute paths and then use only that one. I really don't like having two file resolve functions around. So far I relied on getting deterministic cased absolute paths back from GetExactPathName

[dakotahawkins]

Honestly, I didn't hit a problem-case, but I became very concerned about it because I was adding code that uses naked string operations to compare two different paths.

However, there aren't really two functions now, GetFileSystemCasing is private, and GetExactPathName calls it. I did it that way because the core work is recursive but there's also work you want to do before you start and after you finish, in this case normalizing the separators and adding one to the end of directories respectively.

Ignoring the possibility that the old GetExactPathName might not have normalized the case 100% of the time (again, not positive about that, just worried), the only difference now should be that directory paths will have a trailing separator. That's important because there are (and already were) places where string.StartsWith was used on two paths (prevents accidentally saying that C:\foo\bar.txt is inside of C:\foo\bar\).

[dakotahawkins]

Reading back over the SO comments, I think the old version probably did work. The only thing I'm not 100% sure about is using DI Name instead of FullName. It probably worked fine, since in both cases the DI is built from GetFileSystemInfos(), but I also think the SO people arguing about how to do this did more pedantic/specific testing than you or I.

[dakotahawkins]

The following loops up through parent directories looking for the file specified in the settings.

[dakotahawkins]

For these IgnoreFileRelativeModes, we need to change the path format and root directory to root the include path appropriately.

[Wumpf]

Not very happy with this. The combination of FormatterOptionsPage.IgnoreFileRelativeMode.InSameDirectory/InSameDirectoryOrSubdirectory with FormatterOptionsPage.PathMode.Shortest would no longer find the shortest possible path!
We need to pass on the FormatterOptionsPage.IgnoreFileRelativeMode to the inner FormatPath to be able to find the best path according to the PathMode

[dakotahawkins]

I'll give that a try. I worry a bit that some options will be at cross-purposes with one another in some configurations (like "why would you do that?" configurations). If it makes things less awkward, though, I'm all for it!

[dakotahawkins]

Avoids files outside of the root directory tree (e.g. system includes)

[Wumpf]

[missplaced comment]

[Wumpf]

Let's make this 2.3.0
My thinking here is "major revision"."new features"."bugfixes"

[Wumpf]

Hmm odd. With the newest VS2017 version I'm hitting this as well. Sounds like I should make sure stuff works still in VS2015 before I upload to the marketplace.
Better remove it for good

[Wumpf]

Interesting. I'm not sure I could really follow the discussion on that SO page. Can you give me an example where GetExactPathName gives a different result than GetFileSystemCasing for absolute paths?
Would be really nice if we could use the same function for both relative and absolute paths and then use only that one. I really don't like having two file resolve functions around. So far I relied on getting deterministic cased absolute paths back from GetExactPathName

[Wumpf]

Yeah I never heard of this whole thing and it took me a while indeed. I think your description here is quite good after all though.

Suggestion for a slight improvement:

[DisplayName("Filename for Absolute_FromParentDirWithFile")]
[Description("The Absolute_FromParentDirWithFile mode will look for this file in all parent directories and make include paths absolute from its location if found.")]

[Wumpf]

I haven't checked - what happens if the bool was already saved and were loading an integer now with the same identifier?

[dakotahawkins]

That just happened to me accidentally. The field is blank :(

I guess I could add conversion code, but hacking that in seems like a slippery slope in the event there are more changes like that in the future. What do you think the right thing to do is?

[dakotahawkins]

Actually, it only doesn't work going backwards in version. Going forwards I think it works because the first two enum values correspond to the original boolean values. Of course, that will change if we flip the meaning of the setting.

Also changing the name of the setting, however, should just give you default values. So, not ideal for users that had non-defaults before but maybe not too terrible either.

[Wumpf]

Yeah let's not over-think this (too). Let's go with a different name, then we're on the safe side.

[Wumpf]

Old version was guarded against FormatPath returning null. There might have been a good reason for that.

[dakotahawkins]

It still is, just a few lines down because of how I formatted the parameters. (I dislike commenting on code in GitHub because I find inline comments hard to place well and hard to read -- I'd rather them show up in some kind of side bar so they don't split up the actual code!)

[Wumpf]

This not actually absolute in any sense. It is relative from where the special file is placed, so I'd call it ForceRelativeToParentDirWithFile since it will always try to make the path relative to the special folder, blindly assuming that this will compile. In contrast all the other modes ensure that the the path can be resolved with the IncludePaths in the project file or by being relative to the including source file.

Please correct me if I got this wrong :)

[dakotahawkins]

No, you're right. I think your name will make more sense. In our workflow I just tend to think about them as "absolute with respect to the code base" vs. "relative to the current file".

[Wumpf]

Originally this was intended to suppress the "natural include path" even if that one would be better according to the policy in PathMode.
You're right, this is all not too intuitive. But I think with your extension we completely lost the meaning of "ignore" (never a good description of what was going on). So maybe this would be more concise?

enum AllowRelativePathMode
{
Always, // All relative paths are taken into consideration.
Never, // No relative path is taken into consideration.
OnlyInSameDirectory, // Relative paths are only allowed if they are in the very same directory than the including file.
OnlyInSameOrSubDirectory // Relative paths are allowed if they are in the same directory than the including file or any subdirectory.
}

[dakotahawkins]

I think it might be a better idea to do something different. What do you think about this:

Remove Always/Never

• To resolve the full path to the include, we should automatically use the including file's directory if the delimiters are quotes (https://msdn.microsoft.com/en-us/library/36k2cdd4.aspx)
  • This would go well with what I'd like to do next: Properly handle system includes (adding an Auto to DelimiterFormatting and handling their formatting specially (there's a TODO for something like this already))
• This mode will instead optionally force use of paths relative to the including file's directory, overriding PathFormat if set
  • I really need this behavior -- I want to ForceRelativeToParentDirWithFile unless the include is a system one or in the same directory as the including file
• If this mode is disabled, actually formatting include paths as relative to anything else will be left to the PathFormat

Basically I'm envisioning these two being like this:

public enum PathMode
{
    Unchanged,
    Shortest,               // Maybe for these we can check to see if there's a shorter one than the
    Shortest_AvoidUpSteps,  // force-file-relative mode instead of overriding them entirely 
    Absolute,
    ForceRelativeToParentDirWithFile
}

public enum ForceFileRelativePathMode
{
    Disabled,               // Off
    IfInSameDirectory,      // DEFAULT. Includes found in the directory containing the including
                            // file are always made relative to it. Does not include subdirectories.
    IfInSameOrSubDirectory  // Includes found in the directory containing the including file are
                            // always made relative to it. Includes subdirectories.
}

[Wumpf]

I think you're definitely right on the resolve strategy! Didn't know about "2" for '"' delimiters. Taking this into account would be hard ...

I also agree on most of the rest as well but, as you're pointing out neither Shortest, Shortest_AvoidUpSteps, nor ForceRelativeToParentDirWithFile shouldn't be too much intimidated by ForceFileRelativePathMode. That being said we don't really force relative paths. Also, your suggested setup does not make it clear what you need to do to allow relative paths going outside of the source dir, e.g. "../header.h".
It seems all our modes strive "use the shortest out of the following", only that the list to choose from is differently every time. Looking at things from that perspective we would end up with various bit flags that govern which path are legal for either <> or "". But since this doesn't work with the UI, it might translate to this:

public enum PathFormattingMode
{
    Unchanged,
    Shortest, // default
    ForceAbsolute // absolute, no matter what. for debugging mainly
}
// Relative paths are never considered for includes using <>
public enum PathPool_FileRelativePath
{
    DontConsider, // pretend relative paths are not possible
    Consider, // add relative path like a normal include path
    ConsiderOnlyIfInSameDirectory, // Allow relative only if in same directory
    ConsiderOnlyIfInSameOrSubDirectory, // Allow relative only if in same directory or subdirectory
}
public enum PathPool_ProjectIncludePath
{
   AlwaysConsider // default
   ConsiderOnlyForAngled, // allows you to use enforce using relative or RelativeToParentDirWithFile for quoted ones
}
public enum PathPool_RelativeToParentDirWithFile
{
   DontConsider,
   AlwaysConsider,
   DontConsiderForAngled
}
public bool PathPool_AllowUpSteps = false; // If true allow "../" in directories

If I got everything right that would mean that your new desired mode is configured as Shortest, PathPool_FileRelativePath==ConsiderOnlyIfInSameDirectory , PathPool_ProjectIncludePath==ConsiderOnlyForAngled, PathPool_RelativeToParentDirWithFile==DontConsiderForAngled, PathPool_AllowUpSteps==false
Yeah I know, is this really better? Just trying to find flexible and easy to understand framework that fits everything :-/

About the delimiter thing: Having auto mode sounds great! Just don't rely on any semantic for existing/targeted delimiters, I worked in projects where all delimiters were angle brackets by convention.

[dakotahawkins]

With respect to Shortest and my workflow, I actually want "as long as it needs to be, starting from the root of our repository", but only for files not in the current project (and not for system files, of course).

I've already seen a couple of cases in our codebase where files that are in the current directory had longer relative paths that my current implementation fixed appropriately.

As an example, consider:

• RepoDir/
  • build.root
  • Src/
    • MainProj/
      • MainProj.cpp
      • MainProj.h
      • MainUtility.h
      • stdafx.h
    • ProjA/
      • A_1.h
      • A_2.h
    • ProjB/
      • B_1.h
      • B_2.h

Group regexes:

(?i)stdafx\.h(?-i)                          // PCH
"(?i)$(currentFilename)\.(h|hpp|hxx)(?-i)"  // Corresponding header
<[^.]*\.                                    // C system includes
<                                           // C++ system includes
(\\|/)                                      // Other projects' headers
                                            // The rest (this project's headers)

Original:

// MainProj.cpp
// AdditionalIncludeDirs="../ProjA;../ProjB;$(BuildRootDirMacro);..."

#include "stdafx.h"

#include "A_2.h"
#include "B_1.h"
#include "B_2.h"
#include "MainProj.h"
#include "../Src/ProjA/A_1.h"
#include "../MainProj/MainUtility.h"

#include <vector>
#include <windows.h>
#include <string>

Formatted:

// MainProj.cpp
// AdditionalIncludeDirs="../ProjA;../ProjB;$(BuildRootDirMacro);..."

#include "stdafx.h"

#include "MainProj.h"

#include <windows.h>

#include <vector>
#include <string>

#include "Src/ProjA/A_1.h"
#include "Src/ProjA/A_2.h"
#include "Src/ProjB/B_1.h"
#include "Src/ProjB/B_2.h"

#include "MainUtility.h"

If you're curious, that grouping/order comes from the Google C++ Style Guide, which is what our style guide is based on. I have a few reasons for wanting those long, rooted paths:

1. Standardization. It's super-obvious where each header actually lives.
2. Easy searching
3. Easy parsing with other tools! If you don't have to worry about digging include dirs out of the project files and property sheets, parsing includes becomes much easier.

[dakotahawkins]

In terms of "paths going outside of the source dir", I'll admit that's not a case that affects me personally so I wasn't terribly concerned about it, but I think failing to fit other strategies and falling back on one of the Shortest options or even Unchanged may cover it.

[dakotahawkins]

I really think the wiki page would be good to sort this out, since my intention was to take a step back from the implementation and address what kind of general behavior we'd like to support.

Also, with so many options that affect one another, the possible behaviors really scale. It might be OK if there are some combinations of them that don't make sense and produce bad/silly results as long as those combinations aren't easy to accidentally have and the "good" combinations allow lots of kinds of results that people might actually want or need.

[Wumpf]

Not very happy with this. The combination of FormatterOptionsPage.IgnoreFileRelativeMode.InSameDirectory/InSameDirectoryOrSubdirectory with FormatterOptionsPage.PathMode.Shortest would no longer find the shortest possible path!
We need to pass on the FormatterOptionsPage.IgnoreFileRelativeMode to the inner FormatPath to be able to find the best path according to the PathMode

[Wumpf]

Phew that gave me something to think about :D

The main take away of my comments is that
I think it's best to think of PathMode as a policy (out of the given paths which one is the best) and of AllowRelativePathMode/IgnoreFileRelativeMode as a constraint (which paths are we not allowed to choose).