Add Windows/MSVC support #71

dschmidt · 2018-09-10T00:45:46Z

I tried to be very conservative about changing any logic or performance on other platforms than Windows, that's why I ifdef'ed some code that should be working cross-platform. Moreover I did not want to raise the required C++ version to 2017 for std::filesystem on all platforms. Let me know if you would prefer it that way.

dschmidt · 2018-09-10T01:04:41Z

FWIW here is a blueprint for compiling this with Craft:

import info

class subinfo(info.infoclass):
    def setTargets(self):
        for ver in ["msvc"]:
            self.svnTargets[ver] = f"[git]https://github.com/dschmidt/woboq_codebrowser|{ver}|"

        self.defaultTarget = 'msvc'
        self.description = "Woboq CodeBrowser"
        self.webpage = "http://woboq.com/codebrowser.html"

    def setDependencies(self):
        self.buildDependencies["dev-utils/cmake"] = "default"
        self.buildDependencies["libs/meta-llvm"] = "default"

from Package.CMakePackageBase import *

class Package(CMakePackageBase):
    def __init__(self):
        CMakePackageBase.__init__(self)

I will setup a repository for this at some point, but now I'm tired and excited to get some feedback ;-)

ogoffart

Looks mostly good. Thanks for the work.
Please have a look at inline comments.

ogoffart · 2018-09-10T05:46:14Z

generator/filesystem.cpp

@@ -32,6 +32,11 @@

 #include <iostream>

+// ATTENTION: Keep in sync with ECMAScript function of the same name in common.js
+void replace_invalid_filename_chars(std::string &str)


In Annotator::getReferenceAndTitle, we already escape '<' and '>'. We could also escape ':'

The problem if we do the way you did, is that refs generated with older version of the code browser will not be compatible with the new javascript.

But the problem if we escape it in getReferenceAndTitle, is that URL containing #foo::bar will stop working. And we also do not want that.

So I guess the way you did it is better.

thinking about it, this means that struct Foo__Bar and srutct Foo { struct Bar; }; will have the same filename. Maybe that's not so common that it's not going to clash. (i think double underscore in a identifier is undefined behavior anyway). But could we use a symbol not allowed in a identifier? like . or @ or % or something else. maybe i'd go with .

Sounds reasonable

generator/main.cpp

ogoffart · 2018-09-10T05:54:02Z

generator/filesystem.cpp

@@ -42,14 +52,24 @@ std::error_code canonicalize(const llvm::Twine &path, llvm::SmallVectorImpl<char
    std::string p = path.str();
 #ifdef PATH_MAX
    int path_max = PATH_MAX;
+#elif defined(MAX_PATH)
+    unsigned int path_max = MAX_PATH;
 #else
    int path_max = pathconf(p.c_str(), _PC_PATH_MAX);
    if (path_max <= 0)
        path_max = 4096;
 #endif
    result.resize(path_max);


Well, no need to resize to MAX_PATH when you know the size of the canonical string a bit later.

-> #71 (comment)

No need to resize the buffer or get the MAX_PATH when using llvm::sys::fs::real_path

generator/filesystem.cpp

generator/annotator.cpp

generator/projectmanager.h

CLAassistant · 2018-09-10T09:38:51Z

All committers have signed the CLA.

ogoffart

Consider also checking the demangleFunctionName in the js code.

ogoffart · 2018-09-10T13:13:11Z

generator/filesystem.cpp

@@ -42,14 +52,24 @@ std::error_code canonicalize(const llvm::Twine &path, llvm::SmallVectorImpl<char
    std::string p = path.str();
 #ifdef PATH_MAX
    int path_max = PATH_MAX;
+#elif defined(MAX_PATH)
+    unsigned int path_max = MAX_PATH;
 #else
    int path_max = pathconf(p.c_str(), _PC_PATH_MAX);
    if (path_max <= 0)
        path_max = 4096;
 #endif
    result.resize(path_max);


No need to resize the buffer or get the MAX_PATH when using llvm::sys::fs::real_path

generator/annotator.cpp

ogoffart · 2018-09-10T13:19:53Z

generator/filesystem.cpp

@@ -32,6 +32,11 @@

 #include <iostream>

+// ATTENTION: Keep in sync with ECMAScript function of the same name in common.js
+void replace_invalid_filename_chars(std::string &str)


thinking about it, this means that struct Foo__Bar and srutct Foo { struct Bar; }; will have the same filename. Maybe that's not so common that it's not going to clash. (i think double underscore in a identifier is undefined behavior anyway). But could we use a symbol not allowed in a identifier? like . or @ or % or something else. maybe i'd go with .

ogoffart · 2018-09-10T13:25:54Z

data/common.js

+// ATTENTION: Keep in sync with C++ function of the same name in filesystem.cpp and `Generator::escapeAttrForFilename`
+function replace_invalid_filename_chars(str) {
+    return str.replace(new RegExp(':', 'g'), '_');
+}


would be nice if older generated file would still work with the newer javascript.
older generated files do not include this common.js. So maybe we need a fallback for when the function does not exist.

Note: I've been trying to keep only one .js included. this is why symbol.html has some duplicated code.
I've been wanting to use a build system to concatenate and minify files, but never got around to do that.

I see ... but then references with :: (or now .) still won't work while a lot of other refs still work.

We could possibly set a global var in the html files (:shudder:) that activates the filename sanitizing... old html files won't have it and so we would know not to replace ::.

I hate code duplication with a passion - but for this supershort function, I don't want to invest the time to setup a js build system. Pretty much out of scope for this PR.
Will remove common.js and implement a backwards compatible solution.

Yes, a global var would work. Maybe with a version number or so.

Will maybe be a problem in symbol.html, but that's fine if tooltip there works less.

Ah yes, because that is not generated but shared in the data folder. 🤷

…s again and use . instead of _ as replacement

dschmidt added 4 commits September 9, 2018 16:23

Use unsigned int instead of uint which is not portable

fb31058

Remove : from ref filenames because it's not valid in Windows filenames

532fb4c

Fix typos and make errors more greppable

7a5a28d

Add cctype include to make std::isalnum available on Windows

8a20b07

ogoffart suggested changes Sep 10, 2018

View reviewed changes

dschmidt added 6 commits September 10, 2018 01:50

Fix clang invocation on Windows

abec57b

Implement filesystem functions for Windows

6407315

Convert user supplied paths to forward slashes on Windows

ae7865a

Remove special chars from mangled ref names on Windows

b32a4a9

Do not use compiler and linker flags not supported by MSVC

ea34614

Do not include builtin headers when compiling with MSVC

36adadc

dschmidt force-pushed the msvc branch from ea9cba9 to 99339ee Compare September 10, 2018 09:38

dschmidt force-pushed the msvc branch 2 times, most recently from 71e5ce9 to 5bcaaad Compare September 10, 2018 09:51

dschmidt added 2 commits September 10, 2018 05:23

Move system projects definition to CMake

490b521

Use llvm:sys::fs functions instead of std::filesystem

362ce6c

dschmidt force-pushed the msvc branch from 5bcaaad to 362ce6c Compare September 10, 2018 12:23

ogoffart reviewed Sep 10, 2018

View reviewed changes

dschmidt added 3 commits September 18, 2018 01:56

Actually add system projects

cc61da4

Move replace_invalid_filename_chars from common.js to individual file…

3d8c506

…s again and use . instead of _ as replacement

Rearrange ifdefs in filesystem::canonicalize

abde055

ogoffart merged commit 19f6181 into KDAB:master Sep 18, 2018

Add Windows/MSVC support #71

Add Windows/MSVC support #71

Uh oh!

Conversation

dschmidt commented Sep 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dschmidt commented Sep 10, 2018

Uh oh!

ogoffart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CLAassistant commented Sep 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ogoffart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ogoffart Sep 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dschmidt commented Sep 10, 2018 •

edited

Loading

CLAassistant commented Sep 10, 2018 •

edited

Loading

ogoffart Sep 10, 2018 •

edited

Loading