search

borglab / GTDynamics

Full kinodynamics constraints for arbitrary robot configurations with factor graphs.
BSD 2-Clause "Simplified" License
39 stars 10 forks source link

Replace "" include with <> #341

Closed varunagrawal closed 2 years ago

varunagrawal commented 2 years ago

Fixes #332

varunagrawal commented 2 years ago

I probably should have brought these questions up in RFC, but anyway:

Yes. This is a discussion for that issue (which is why I created it and tagged it as an RFC).

where syntax (1) is <>. But this makes me think that only c/c++ std libs should use <> and everything else should use ""?

Not necessarily. The use of angle brackets is to tell the compiler/linker where to look for specific headers. If we release GTDynamics as a library, we want the compiler looking in the standard install directories as well and not limiting itself to the project directory. Relevant SO answer.

  • Eigen (just the first "proper library" example I happened to think of) uses the "" syntax for its own files
  • Boost I wasn't able to recognize a clear pattern, but it appears to use the "" syntax for files in the same "lib", and uses the <> syntax only for files in a different lib (e.g. boost optional)

This seems to be a matter of choice for the developers. Generally if something isn't a part of the external API, it would be fine to include it with "".

One practical issue I would be concerned about is I recall once I had something set up wrong and my code was using the installed version of the include instead of the local version, so when I was changing the local version everything compiled fine but then didn't make the changes I was expecting. I'm not sure if changing to "" would help at all (since it falls back to <>), but might be more clear it's local?

This is a CMake issue where it seems the variables weren't set up correctly.

It seems to me the primary advantage of using <> is that it is less likely to collide with files in projects that include gtdynamics, but I doubt this is much of a concern since all the includes start with "gtdynamics/..." so it doesn't seem like there's any real advantage of using <> ?

The points you raised are a bit irrelevant because you're considering GTDynamics development itself rather than using it as an external library (I may be wrong but your wording seems to indicate this). If GTDynamics was released by company X, you would want it to install in the correct locations and the compiler to know where to look without specifying specific flags like -I. It's the same reason we use <> in GTSAM and not "".

yetongumich commented 2 years ago

I think we shall merge this in.

varunagrawal commented 2 years ago

@gchenfc should we merge this in?

gchenfc commented 2 years ago

Sure, sorry I thought I saw an email that said this was already merged but I must have misread.