ilmola / generator

A procedural geometry generation library for C++11
GNU Lesser General Public License v2.1
211 stars 26 forks source link

Make generator more msvc-friendly #19

Closed JesseTG closed 8 years ago

JesseTG commented 8 years ago

This PR makes generator able to compile in Visual C++ 2015 by doing the following:

ilmola commented 8 years ago

Why did you change private inheritance to public? Private inheritance is an implementation detail. Public inheritance is part of the interface.

JesseTG commented 8 years ago

MSVC was choking on the private inheritance for some reason (saying it couldn't access the members specified with the using declarations), while gcc and clang took it just fine. I'm not sure which is the correct behavior; I think this might be up to different interpretations of the C++ standard. What do you want me to do?

ilmola commented 8 years ago

Private inheritance could be changed to private composition. Here is a branch created by search replacing inheritance to composition:

https://github.com/ilmola/generator/tree/inheritance_to_composition

Does it compile with MSVC?

JesseTG commented 8 years ago

I'll get back to you on that tonight (for you, tomorrow). It's probably the better thing to do anyway, because now there's not as much indirection or other virtual fuckery.

JesseTG commented 8 years ago

@ilmola Yes it does, actually. Haven't tried it with gcc or clang, though. After I do, assuming it checks out do you want me to cherry-pick the relevant commits onto this PR?

Also, can I see the script you wrote to process the code base? I'm curious.

ilmola commented 8 years ago

I am not familiar with git best practices, but it seems that all you need to do, is to merge your chances to the cmake files to the version generated by the script, and then you can submit a version that compiles with MSVC and does not introduce a massive API change.

The script was just this quick and dirty php search and replace. It converts all but 2 lines that need to fixed by hand.

function convertFile($file, $path, $pathNew) {
    $content = file_get_contents($path."include/generator/".$file);

    $srcFile = str_replace("hpp", "cpp", $file);
    $srcPath = $path."src/".$srcFile;
    $srcContent = file_exists($srcPath) ? file_get_contents($srcPath) : "";

    $matches = [];
    preg_match_all("/class\s+(.+)\s+(:\s+private\s([^{]+)){/", $content, $matches);

    for ($i = 0; $i < count($matches[0]); $i++) {
        $parent = trim($matches[3][$i]);
        $shortParent = explode("<", $parent)[0];
        $member = lcfirst($shortParent)."_";

        $newClass = "class ".$matches[1][$i]."\n{\nprivate:\n\n\tusing Impl = ".$parent.";\n\tImpl ".$member.";\n"; 
        $content = str_replace($matches[0][$i], $newClass, $content);

        foreach (["triangles", "edges", "vertices"] as $t) {
            $T = ucfirst($t);
            $newFn = "using ".$T." = typename Impl::".$T.";\n\n\t".$T." ".$t."() const noexcept { return ".$member.".".$t."(); }";

            $content = str_replace("using ".$shortParent."::".$t.";", $newFn, $content);
            $content = str_replace("using ".$parent."::".$t.";", $newFn, $content);
        }

        foreach (["mesh", "path", "shape"] as $name) {
            $cast = "static_cast<const ".$parent."&>(".$name.")";
            $content = str_replace($cast, $name.".".$member, $content);
        }

        $content = str_replace($parent."{", $member."{", $content);
        $srcContent = str_replace($parent."{", $member."{", $srcContent);
        $srcContent = str_replace($shortParent."{", $member."{", $srcContent);  
    }

    file_put_contents($pathNew."include/generator/".$file, $content);

    if ($srcContent != "") file_put_contents($pathNew."src/".$srcFile, $srcContent);
}

function convertDir($path, $pathNew) {
    foreach (glob($path."include/generator/*") as $filePath)
        convertFile(basename($filePath), $path, $pathNew);
}

convertDir($argv[1], $argv[2]);
JesseTG commented 8 years ago

I'll just cherry-pick the commit, then. However, I still want to ensure that this won't give gcc or clang a hard time. I also have a linker problem and I need to figure out if it's related to this or not (probably not but I don't want to merge bad code into master).

I don't want to create a whole new branch or PR because not all the changes in this one are about the code itself (the stuff in CMakeLists.txt, for instance, is vitally important).

JesseTG commented 8 years ago

That branch is good to go with gcc. Can't try it with clang right now because of an unrelated problem. I'll cherry-pick the relevant commits onto this branch.