Skip to content

CppCodeWriter should write out paths using / for the directory separator instead of \ #165

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
PathogenDavid opened this issue Feb 25, 2021 · 3 comments
Labels
Area-OutputGeneration Issues concerning the process of generating output from Biohazrd Concept-OutputFriendliness Issues concerning the friendliness of using the Biohazrd output RelativelySmall Issues that are relatively small and could be good candidates for someone's first issue.

Comments

@PathogenDavid
Copy link
Member

No description provided.

@PathogenDavid PathogenDavid added Area-OutputGeneration Issues concerning the process of generating output from Biohazrd Concept-OutputFriendliness Issues concerning the friendliness of using the Biohazrd output RelativelySmall Issues that are relatively small and could be good candidates for someone's first issue. labels Feb 25, 2021
@PathogenDavid
Copy link
Member Author

Reopening this because it came up again for Linux support #108 since IncludePathsAreNormalizedToForwardSlash1 broke.

Right now the logic added by this change is Windows-only. This is technically correct, but might be problematic.

A backslash is technically a valid character for file/folder names on Linux, but they are definitely not common.

This creates an awkward situation where a poorly-written generator might work fine on Windows but not on Linux if the generator author is building include paths using string concatenation instead of Path.Combine.

We essentially have three options:

  • Do nothing
    • This is technically the most correct option.
    • A poorly-written generator could have issues running on Linux when it works fine on Windows. The issue might end up being non-obvious.
    • The output in theory differs depending on the host OS irregardless of any attempts to do proper cross-compilation. (Although maybe this doesn't matter since anyone smart enough to get proper cross-compilation going would be aware of path differences.)
  • Replace \ with / even on Linux
    • This is similar to what MSBuild does (IIRC). It handles poorly-written Windows developer code at the expense of not handling the legitimate Linux edge case.
  • Throw on Linux when attempting to include a header path with a backslash.
    • Poorly-written generators still fail, but they fail earlier and hopefully in a more-obvious manner.

@PathogenDavid
Copy link
Member Author

I created a test to see how C++ compilers react to this "legitimate" situation on Linux to see if maybe they print a really obvious message about how you're trying to compile code written by an ignorant Windows dev.

First: Lol, VSCode barfs trying to open a \ file. (Trying to run code Dumb\\Name.h causes VSCode to open a new instance with no files open.)

To test I created this file:

#include "Dumb\Name.h"

int Test(MyStruct s)
{
    return s.x;
}

int main()
{
    MyStruct s;
    return Test(s);
}

in one case I created the file in the same directory Dumb\Test.h (the legitimate edge case) and in the other case I crated the file Dumb/Test.h (the bad Windows dev case).:

struct MyStruct
{
    int x;
};

For the sake of clarity, here's the directory tree:

pathogendavid@lovebuntu:~/Playground/IncludeEvilFiles$ tree
.
├── IncludeIsRight
│   ├── a.out
│   ├── Dumb\Name.h
│   └── Main.cpp
└── IncludeIsWrong
    ├── Dumb
    │   └── Name.h
    └── Main.cpp

For the legitimate edge case, neither GCC nor Clang complained.

For the bad Windows dev case, both obviously complain but do not mention anything specific about the file existing if the path separator wasn't dumb (which isn't surprising, but I had hope):

pathogendavid@lovebuntu:~/Playground/IncludeEvilFiles/IncludeIsWrong$ clang Main.cpp
Main.cpp:1:10: fatal error: 'Dumb\Name.h' file not found
#include "Dumb\Name.h"
         ^~~~~~~~~~~~~
1 error generated.
pathogendavid@lovebuntu:~/Playground/IncludeEvilFiles/IncludeIsWrong$ gcc Main.cpp
Main.cpp:1:10: fatal error: Dumb\Name.h: No such file or directory
    1 | #include "Dumb\Name.h"
      |          ^~~~~~~~~~~~~
compilation terminated.

@PathogenDavid
Copy link
Member Author

I think the original behavior is the most correct. The intent behind this change was to make it so Path.Combine("A", "B.h") emitted a portable #include directive on Windows, not to correct for shoddy path concatenation. I also don't like the idea of blocking a valid Linux situation even if it is rare and terrible.

We can revisit this if it ever becomes an issue. However I suspect the target audience of Biohazrd is generally going to know that directory + "\\" + fileName is the wrong way to concatenate paths.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-OutputGeneration Issues concerning the process of generating output from Biohazrd Concept-OutputFriendliness Issues concerning the friendliness of using the Biohazrd output RelativelySmall Issues that are relatively small and could be good candidates for someone's first issue.
Projects
None yet
Development

No branches or pull requests

1 participant