Resolved several MSVC compiling errors #87

cntaylor · 2019-07-13T00:52:17Z

The check.base unit tests all pass now.
The gstam_matlab_wrapper class compiles with now errors now.
Note that I had to remove all LieMatrix, LieVector, and LieScalar stuff
to get this to work...

This change is

* The check.base unit tests all pass now. * The gstam_matlab_wrapper class compiles with now errors now. * Note that I had to remove all LieMatrix, LieVector, and LieScalar stuff to get this to work...

cntaylor · 2019-07-13T02:14:47Z

Several of the "check." libraries seem to have multiple defined test functions. However, I was able to --using this branch and commit -- compile gtsam_toolbox and the examples seem to run in MATLAB, so I think it is all compiling properly. This happens despite the check.geometry and check.inference projects, for example, not compiling properly. Happy that I have been able to compile a MATLAB toolbox from source though. Now to add my own factors... :)

dellaert

Thanks for your fix. However, it's a really heavy-handed fix. Basically, you're saying: this feature does not work on Windows, we'll just remove it. However, while LieXXX are deprecated in GTSAM 4, it was consciously not removed to allow any project that was still referring to these classes to still work. This is also why the tests are still there. This is a great PR for GTSAM 5, where we will simply remove it, but not for GTSAM 4.

To fix the linking error, how about adding dummy *.cpp files instead? It's a bit weird to me though, because I believe there are other header-only classes. So I'm not 100% sure I believe your reasoning that it is the lack of cpp files that is causing the issue.

cntaylor · 2019-07-13T10:18:56Z

It didn't seem like the optimal fix. However, all of the errors are of the type that "you have declared a function, but there is no definition for it", very similar to what would happen if you have the .h files but not the .cpp files in a normal circumstance. (Honestly, I was wondering if someone had removed the .cpp files when they deprecated, but not fully removed the requirement for that code from all sections of the repository.) Are the .h files supposed to be the full implementation of the Lie* classes?

dellaert · 2019-07-13T10:22:07Z

You can answer that question :-) Just check which methods VS complains about have no body in the .h file?

dellaert · 2019-07-13T10:23:12Z

BTW: the fact it compiles and links on mac/linux makes me think: none...

cntaylor · 2019-07-13T11:28:45Z

Unfortunately... I think you are right. I can poke at this some more on Monday.

…

On Sat, Jul 13, 2019, 6:23 AM Frank Dellaert ***@***.***> wrote: BTW: the fact it compiles *and* links on mac/linux makes me think: none... — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#87?email_source=notifications&email_token=ABU4H73ZCW2PSGL3Q32JIZDP7GUJFA5CNFSM4ICUAXAKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZ3O5LI#issuecomment-511110829>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABU4H7ZWF2GLVWAODYTDPTLP7GUJFANCNFSM4ICUAXAA> .

This reverts commit 33bda1e.

cntaylor · 2019-07-14T02:38:33Z

Well, I think I finally understand how this dllexport and dllimport stuff works with MSVC. Made the fix rather simple, actually. Take a look at this one and see what you think...

dellaert

OK, this is getting closer :-) One thing I still don't understand is why the switch from class-level EXPORT to method-level EXPORT for PointX.h. And, why no EXPORT for Lie classes? Since you say you figured it out, I propose you add a (short) comment in all those classes as to why it is so.
Finally, please note there is a compile-time switch to typedef Point2 and Point3 to Eigen vectors. Would it be too much trouble (I know windows has a very slow compile!) to check whether everything still works under that flag?

dellaert

Awesome! Since you're merging into José's PR, I propose he merges when he validates this builds on his machine. @jlblancoc OK?

jlblancoc · 2019-07-15T22:13:47Z

Sure, @dellaert !

Using-GTSAM-EXPORT.md

jlblancoc · 2019-07-15T22:17:46Z

Using-GTSAM-EXPORT.md

@@ -0,0 +1,41 @@
+# Using GTSAM_EXPORT
+


wow, this doc is like a large Blog entry on the topic... good idea, @dellaert , @cntaylor ! 👍

jlblancoc · 2019-07-15T22:18:37Z

Using-GTSAM-EXPORT.md

@@ -0,0 +1,41 @@
+# Using GTSAM_EXPORT
+
+To create a DLL in windows, the `GTSAM_EXPORT` keyword has been created and needs to be applied to different methods and classes in the code to expose this code outside of the DLL.  However, there are several intricacies that make this more difficult than it sounds.  In general, if you follow the following three rules, GTSAM_EXPORT should work properly.  The rest of the document also describes (1) the common error message encountered when you are not following these rules and (2) the reasoning behind these usage rules.


the GTSAM_EXPORT keyword has been created

Should it be clearer mentioning that it is CMake the one who automatically defines this symbol (as far as I know) when building dynamic libs?

Using-GTSAM-EXPORT.md

jlblancoc · 2019-07-15T22:24:58Z

Using-GTSAM-EXPORT.md

+
+Rule #1 doesn't seem very bad, until you combine it with rule #2
+
+***Compiler Rule #2*** Anything defined in a header file is not included in a DLL.


Anything defined in a header file is not included in a DLL.

I'm not 100% sure of this... it is NOT exported if you mark it explicitly with inline. Otherwise, I'm not sure if there is a well-defined, standardized expected behavior (?).
(Edit: I mean, if one has:

class GTSAM_EXPORT Foo { public: void doit(int &a) { a++; } };

would doit() be exported? I think it will...
).

I'll kindly request the knowledge of @jolting here just in case he can help us to be accurate in the description...
@jolting

Please note that my expertise on DLLimport/export is what I have learned playing with GTSAM for the last week or so getting it to compile and looking up things online. So, I am theorizing to the best of my knowledge, but I certainly am not an expert.

That said, I have seen several website mention that if you declare a function in a header inside a class definition, the compiler automatically makes it an "inline" function. If it was not inline and you included this class definition in more than one file, then you would get a "multiple definitions of the function 'doit'" error. So this one is essentially marked inline and will not be put in the DLL.

Once again, I should probably change "define" to declare. My im-precision may be causing confusion. :(

I guess it may be possible that a non-inline function is declared in a header file and included in the DLL, but that seems like a corner case (due to the "multiple instances" compilation problem inherent with non-inline functions) so I don't think we need to worry about that too much.

Using-GTSAM-EXPORT.md

jlblancoc · 2019-07-18T05:47:44Z

I verified it builds, thanks @cntaylor ! I'll merge, please open a new PR if you want to add new fixes 👍

jlblancoc · 2019-07-18T06:09:06Z

@cntaylor I noticed that there is an inconsistent usage of export macros between GTSAM and GTSAM_UNSTABLE. I'll fix it in the branch msvc_fixes. Please, update your local copy of that branch if you also keep working on that...

Resolved several MSVC compiling errors

33bda1e

* The check.base unit tests all pass now. * The gstam_matlab_wrapper class compiles with now errors now. * Note that I had to remove all LieMatrix, LieVector, and LieScalar stuff to get this to work...

cntaylor mentioned this pull request Jul 13, 2019

(WIP) Changes to get gtsam to compile in Windows #85

Merged

2 tasks

cntaylor mentioned this pull request Jul 13, 2019

Linking errors on MSVC (2017 Community) #86

Closed

dellaert requested changes Jul 13, 2019

View reviewed changes

cntaylor added 2 commits July 13, 2019 22:08

Revert "Resolved several MSVC compiling errors" -- bring back Lie*

063cf72

This reverts commit 33bda1e.

Fixed compilation issues for MSVC -- for real this time.

44faca1

More GTSAM_EXPORT fixes. This allows gtsam_unstable to compile

ffb3043

dellaert reviewed Jul 14, 2019

View reviewed changes

Added document explaining GTSAM_EXPORT usage rules

e28fb84

dellaert requested a review from jlblancoc July 15, 2019 16:13

dellaert approved these changes Jul 15, 2019

View reviewed changes

jlblancoc requested changes Jul 15, 2019

View reviewed changes

cntaylor added 2 commits July 15, 2019 20:19

Reducing errors in check* libraries when compiling

a49ed61

Updated Using GTSAM_EXPORT document

393e9ed

jlblancoc merged commit 9f9cff4 into borglab:msvc-fixes Jul 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolved several MSVC compiling errors #87

Resolved several MSVC compiling errors #87

cntaylor commented Jul 13, 2019 •

edited by dellaert

Loading

cntaylor commented Jul 13, 2019

dellaert left a comment

cntaylor commented Jul 13, 2019

dellaert commented Jul 13, 2019

dellaert commented Jul 13, 2019

cntaylor commented Jul 13, 2019 via email

cntaylor commented Jul 14, 2019

dellaert left a comment

dellaert left a comment

jlblancoc commented Jul 15, 2019

jlblancoc Jul 15, 2019

jlblancoc Jul 15, 2019

jlblancoc Jul 15, 2019 •

edited

Loading

cntaylor Jul 16, 2019

jlblancoc commented Jul 18, 2019

jlblancoc commented Jul 18, 2019

		@@ -0,0 +1,41 @@
		# Using GTSAM_EXPORT

		To create a DLL in windows, the `GTSAM_EXPORT` keyword has been created and needs to be applied to different methods and classes in the code to expose this code outside of the DLL. However, there are several intricacies that make this more difficult than it sounds. In general, if you follow the following three rules, GTSAM_EXPORT should work properly. The rest of the document also describes (1) the common error message encountered when you are not following these rules and (2) the reasoning behind these usage rules.


		Rule #1 doesn't seem very bad, until you combine it with rule #2

		*Compiler Rule #2* Anything defined in a header file is not included in a DLL.

Resolved several MSVC compiling errors #87

Resolved several MSVC compiling errors #87

Conversation

cntaylor commented Jul 13, 2019 • edited by dellaert Loading

cntaylor commented Jul 13, 2019

dellaert left a comment

Choose a reason for hiding this comment

cntaylor commented Jul 13, 2019

dellaert commented Jul 13, 2019

dellaert commented Jul 13, 2019

cntaylor commented Jul 13, 2019 via email

cntaylor commented Jul 14, 2019

dellaert left a comment

Choose a reason for hiding this comment

dellaert left a comment

Choose a reason for hiding this comment

jlblancoc commented Jul 15, 2019

jlblancoc Jul 15, 2019

Choose a reason for hiding this comment

jlblancoc Jul 15, 2019

Choose a reason for hiding this comment

jlblancoc Jul 15, 2019 • edited Loading

Choose a reason for hiding this comment

cntaylor Jul 16, 2019

Choose a reason for hiding this comment

jlblancoc commented Jul 18, 2019

jlblancoc commented Jul 18, 2019

cntaylor commented Jul 13, 2019 •

edited by dellaert

Loading

jlblancoc Jul 15, 2019 •

edited

Loading