Create support for auto converting includes to imports. #43

Mitten-O · 2015-11-01T10:35:32Z

Added command line options
--import-filter
A regular expression. All includes matching this will be converted to imports.
Only the basename part of the file name is used to generate the import.
--import-prefix
A prefix to give the imports. The import will have the form ""

This could be extended in the future to support filter-prefix pairs to give different prefixes to
includes from different paths, but this was all I needed for now.

jacob-carlborg · 2015-11-01T11:26:12Z

The existing tests break
There are no tests for this feature
The code doesn't follow the existing style. The new code isn't even consistent in it self

Mitten-O · 2015-11-02T19:45:22Z

Sorry, I had trouble noticing the tests (I even tried to look for some, though not very hard), being unfamiliar with cucumber.

I created one now for the change I made and fixed what broke. Is one sufficient or should I maybe test the defaults or something like that?

I also tried to align my white space with your style. I'm a bit blind to it, though.

jacob-carlborg · 2015-11-02T19:47:44Z

dstep/translator/IncludeHandler.d

@@ -109,21 +123,43 @@ class IncludeHandler
        imports ~= "core.stdc.config";
    }

+    /// Makes includes that match regex filter be converted to import with prefix.
+    void setAutoImportPrefix (string prefix)


Please use properties instead of Java style setters.

jacob-carlborg · 2015-11-02T20:17:47Z

I created one now for the change I made and fixed what broke. Is one sufficient or should I maybe test the defaults or something like that?

There is already a simple test for includes. Not sure if that's enough.

jacob-carlborg · 2015-11-02T20:19:38Z

dub.json

@@ -19,7 +19,7 @@
            "targetName": "dstep",
            "sourcePaths": ["dstep", "clang"],
            "importPaths": ["dstep", "clang"],
-            "lflags-posix": ["-lclang", "-rpath", ".", "-L."],
+            "lflags-posix": ["-lclang","-lz", "-rpath", ".", "-L."],


I don't understand why this is necessary.

I got linker errors from tango without it. Ubuntu 15.04, ldc 0.14.0.

That's weird. Can you see in the linker error which function is using libz? Could you please try with DMD as well. I've never compiled DStep with LDC.

Here is the full output:

The determined compiler type "ldc" doesn't match the expected type "dmd". This will probably result in build errors.
Target tango 1.0.1+2.067 is up to date. Use --force to rebuild.
Target mambo 0.0.4 is up to date. Use --force to rebuild.
Target dstack 0.0.3 is up to date. Use --force to rebuild.
Building dstep 0.1.1+commit.34.g582f71b configuration "default", build type debug.
Running ldmd2...
/usr/bin/ld: ../../../.dub/packages/tango-1.0.1_2.067/libtango.a(libtango.o): undefined reference to symbol 'inflateInit2_'
/lib/x86_64-linux-gnu/libz.so.1: error adding symbols: DSO missing from command line
collect2: error: ld returned 1 exit status
Error: /usr/bin/gcc failed with status: 1
FAIL .dub/build/default-debug-linux.posix-x86_64-ldc_2065-16CE78E47BDF9B160EF11DDF27A84EA1/ dstep executable
Error executing command build:
ldmd2 failed with exit code 1.

Seems to be missing inflateInit2.

Works fine without it when using dmd, but isn't this a small price for supporting ldc (or Ubuntu, or whatever has the bug that causes this)?

Mitten-O · 2015-11-06T19:33:03Z

There is already a simple test for includes. Not sure if that's enough.
The added test checks that imports are automatically generated from included files.

The imports are actually generated only for headers whose types are actually used in the file. This is arguably unintuitive, but it works for my needs and was the most fluent way to add it to the existing logic. I added a note about this in the dummy include README.

jacob-carlborg · 2015-11-07T16:22:14Z

dstep/translator/IncludeHandler.d

@@ -109,12 +121,39 @@ class IncludeHandler
        imports ~= "core.stdc.config";
    }

+    @property{


I would prefer that you put the @property attribute on each method.

jacob-carlborg · 2015-11-07T16:56:41Z

The imports are actually generated only for headers whose types are actually used in the file. This is arguably unintuitive, but it works for my needs and was the most fluent way to add it to the existing logic

One could argue that C includes are not very intuitive 😃. The problem with C includes is that they're not reliable. Often includes are missing because they're included implicitly through other includes. In some cases it's not even possible to add all required includes even if you wanted to. I have seen header files that don't include a single header file yet they still use types declared in other files.

Instead DStep looks at type (or similar) that is used in a header file and asks Clang where it's declared. Then it maps that answer from Clang to a corresponding D module.

Mitten-O · 2015-11-08T09:14:57Z

I moved the attribute to its proper place. Yes, that looks much nicer. Thanks for bearing with me, I'm new to D. Even worse, because of my job, I'm pretty steeped in the dark age logic of c++.

BTW, if and when I finish polishing this to the standards of the project, would you like it to be rebased into a neater story or do you want to keep the full journey?

jacob-carlborg · 2015-11-08T12:40:17Z

BTW, if and when I finish polishing this to the standards of the project, would you like it to be rebased into a neater story or do you want to keep the full journey?

In this case I think it's best to squash all commits to one.

jacob-carlborg · 2015-11-08T13:54:56Z

dstep/translator/IncludeHandler.d

@@ -133,11 +172,33 @@ private:

    string isKnownInclude (string include)
    {
-        include = Path.stripExtension(include);
+        include = Path.stripExtension (include);


There should be no space between the function name and the opening parenthesis.

Added command line options --import-filter A regular expression. All includes matching this will be converted to imports. Only the basename part of the file name is used to generate the import. --import-prefix A prefix to give the imports. The import will have the form "<prefix>.<basename>" Includes a simple test case.

Mitten-O · 2016-01-18T20:27:21Z

I'm back! I fixed the latest style deviations and squashed everything together. Also, it seems with the updates you have done, the need for the "lz" flag is gone so I got rid of that addition. So good updates, thanks.

jacob-carlborg · 2016-01-19T19:29:45Z

I have been thinking about this. I'm wondering if it's possible to automatically translate all includes as long as the files are nested within a given root directory. Given a directory layout as:

foo
  |- bar
  |   | - a.h
  |
  b.h

And b.h containing: #include "bar/a.h". When running DStep like dstep b.h --root foo, b.h would be translated to import foo.bar.a;, because a.h is located inside the given root, foo. Thoughts?

Mitten-O · 2016-01-19T21:20:45Z

You mean to actually look for the includes in the file system and run dstep on them, too? That sounds rather useful. For projects that have one master header that includes the rest, one could translate the whole thing with one command.

My main interest lies in wrapping libgit2 for D (the available bindings are a bit outdated) and their header structure is fairly flat, so nested translation is not entirely necessary for my needs, though.

The other interpretation of what you suggest is to just translate the names, not reading the file system or anything, just converting includes that start with the root name to dotted form. That would be simpler to implement and certainly help already.

Either way a bit beyond the scope of this pull request. It's perfectly fine if you choose to reject this if my changes are in the way of some proper vision for include conversion.

jacob-carlborg · 2016-01-20T07:38:07Z

You mean to actually look for the includes in the file system and run dstep on them, too? That sounds rather useful. For projects that have one master header that includes the rest, one could translate the whole thing with one command.

No, that's not what I'm thinking.

The other interpretation of what you suggest is to just translate the names, not reading the file system or anything, just converting includes that start with the root name to dotted form. That would be simpler to implement and certainly help already.

That's kind of what I'm thinking.

DStep would continue to only add imports for what's used. The include directives are not reliable since includes are basically like public imports in D. I've also seen header files without a single include directive, yet they refer to symbols not present in the header file.

The idea of the --root flag is to get the top level package in D, and all the nested packages. For any symbol that is referenced in a header file that is declared in some other header file, which path is included in the root directory, specified with the --root flag, will be translated to an import statement.

Not sure if that explanation clarifies anything 😃. Would that be of any help for you?

Either way a bit beyond the scope of this pull request. It's perfectly fine if you choose to reject this if my changes are in the way of some proper vision for include conversion.

I don't think what I have in mind is any more complicated than this pull request (I hope 😃). I'm not saying that you need to implement my suggestion instead.

Mitten-O · 2016-01-20T17:22:25Z

Ah, I think I get you now. In cases where there are no subfolders in foo, your dstep b.h --root foo would be equivalent with my dstep b.h --include-filter "foo/.*" --include-prefix "foo". It would have nesting, which my solution does not, and be more semantic, but less powerful than regexes. You're right, it does not sound too complicated. But it is a solution to the same problem my changes are trying to solve, so if you do go that way, this pull request will be obsolete.

Mitten-O force-pushed the include2import branch 2 times, most recently from e381fe4 to 7f28f12 Compare November 2, 2015 19:35

jacob-carlborg reviewed Nov 2, 2015
View reviewed changes

jacob-carlborg reviewed Nov 7, 2015
View reviewed changes

jacob-carlborg reviewed Nov 8, 2015
View reviewed changes

Mitten-O force-pushed the include2import branch from 582f71b to 32619d0 Compare January 18, 2016 18:53

jacob-carlborg force-pushed the master branch from 0e17165 to 4b897a1 Compare July 1, 2017 08:37

jacob-carlborg force-pushed the master branch from 1ab3153 to 8bb5788 Compare April 17, 2019 10:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create support for auto converting includes to imports. #43

Create support for auto converting includes to imports. #43

Mitten-O commented Nov 1, 2015

jacob-carlborg commented Nov 1, 2015

Mitten-O commented Nov 2, 2015

jacob-carlborg Nov 2, 2015

jacob-carlborg commented Nov 2, 2015

jacob-carlborg Nov 2, 2015

Mitten-O Nov 6, 2015

jacob-carlborg Nov 7, 2015

Mitten-O Nov 8, 2015

Mitten-O commented Nov 6, 2015

jacob-carlborg Nov 7, 2015

jacob-carlborg commented Nov 7, 2015

Mitten-O commented Nov 8, 2015

jacob-carlborg commented Nov 8, 2015

jacob-carlborg Nov 8, 2015

Mitten-O commented Jan 18, 2016

jacob-carlborg commented Jan 19, 2016

Mitten-O commented Jan 19, 2016

jacob-carlborg commented Jan 20, 2016

Mitten-O commented Jan 20, 2016

Create support for auto converting includes to imports. #43

Are you sure you want to change the base?

Create support for auto converting includes to imports. #43

Conversation

Mitten-O commented Nov 1, 2015

jacob-carlborg commented Nov 1, 2015

Mitten-O commented Nov 2, 2015

jacob-carlborg Nov 2, 2015

Choose a reason for hiding this comment

jacob-carlborg commented Nov 2, 2015

jacob-carlborg Nov 2, 2015

Choose a reason for hiding this comment

Mitten-O Nov 6, 2015

Choose a reason for hiding this comment

jacob-carlborg Nov 7, 2015

Choose a reason for hiding this comment

Mitten-O Nov 8, 2015

Choose a reason for hiding this comment

Mitten-O commented Nov 6, 2015

jacob-carlborg Nov 7, 2015

Choose a reason for hiding this comment

jacob-carlborg commented Nov 7, 2015

Mitten-O commented Nov 8, 2015

jacob-carlborg commented Nov 8, 2015

jacob-carlborg Nov 8, 2015

Choose a reason for hiding this comment

Mitten-O commented Jan 18, 2016

jacob-carlborg commented Jan 19, 2016

Mitten-O commented Jan 19, 2016

jacob-carlborg commented Jan 20, 2016

Mitten-O commented Jan 20, 2016