search

modelon-community / fmi-library

C library for importing FMUs
Other
115 stars 34 forks source link

Parse Binary and Clock variables #39

Closed filip-stenstrom closed 1 year ago

filip-stenstrom commented 1 year ago
filip-stenstrom commented 1 year ago

Looks very good!

Some overall comments not bound to any specific code segment:

  1. Comment style // vs / / is a bit inconsistent. Is this worthwhile fixing? Does this need to be C89 compliant?
  2. There are quite a few TODOs, XXX, and 1 BUG in comments. Are we keeping proper track of all of these, are theses expected to be resolved within this MR?
  3. There are a quite a few "User is responsible for freeing memory etc comments" I think it could be useful provide the how, since it might not be clear how to do this. And even if it is a simple free(...), this may not be obvious either.

Thanks for review. Great that you managed to find some stuff even when haven't seen code before!

  1. I now do // for comments in code, because that's more convenient, and /* / for documentation. I think that's how we should do it moving forward. We're not C89 compliant any longer due to the new variable types, which require stdint.h and stdbool.h which are C99. I still try to avoid // comments in API headers though if any user would try to macroify out the new types.
  2. No, I don't track them externally. Unless it clearly should be fixed in the scope of the PR, I think we should go through them when we got a minimal working base for FMI3.
  3. It's always some kind of _list object, and their API has a free method. A user should never use pure free. I think it's more of a problem that we're not consistent or even documenting everywhere when a user needs to handle memory
PeterMeisrimelModelon commented 1 year ago

Looks very good! Some overall comments not bound to any specific code segment:

  1. Comment style // vs / / is a bit inconsistent. Is this worthwhile fixing? Does this need to be C89 compliant?
  2. There are quite a few TODOs, XXX, and 1 BUG in comments. Are we keeping proper track of all of these, are theses expected to be resolved within this MR?
  3. There are a quite a few "User is responsible for freeing memory etc comments" I think it could be useful provide the how, since it might not be clear how to do this. And even if it is a simple free(...), this may not be obvious either.

Thanks for review. Great that you managed to find some stuff even when haven't seen code before!

  1. I now do // for comments in code, because that's more convenient, and /* / for documentation. I think that's how we should do it moving forward. We're not C89 compliant any longer due to the new variable types, which require stdint.h and stdbool.h which are C99. I still try to avoid // comments in API headers though if any user would try to macroify out the new types.
  2. No, I don't track them externally. Unless it clearly should be fixed in the scope of the PR, I think we should go through them when we got a minimal working base for FMI3.
  3. It's always some kind of _list object, and their API has a free method. A user should never use pure free. I think it's more of a problem that we're not consistent or even documenting everywhere when a user needs to handle memory
  1. How do we stay somewhat consistent with this. Any comment is a sort of documentation. Or do you propose // for in-line comments vs / ... / anything going into doxygen?
  2. Seems reasonable.
  3. Sounds like it should be reasonably easy to find the appropriate free method then and I suppose we'll improve documentation as we go then.
filip-stenstrom commented 1 year ago

Looks very good! Some overall comments not bound to any specific code segment:

  1. Comment style // vs / / is a bit inconsistent. Is this worthwhile fixing? Does this need to be C89 compliant?
  2. There are quite a few TODOs, XXX, and 1 BUG in comments. Are we keeping proper track of all of these, are theses expected to be resolved within this MR?
  3. There are a quite a few "User is responsible for freeing memory etc comments" I think it could be useful provide the how, since it might not be clear how to do this. And even if it is a simple free(...), this may not be obvious either.

Thanks for review. Great that you managed to find some stuff even when haven't seen code before!

  1. I now do // for comments in code, because that's more convenient, and /* / for documentation. I think that's how we should do it moving forward. We're not C89 compliant any longer due to the new variable types, which require stdint.h and stdbool.h which are C99. I still try to avoid // comments in API headers though if any user would try to macroify out the new types.
  2. No, I don't track them externally. Unless it clearly should be fixed in the scope of the PR, I think we should go through them when we got a minimal working base for FMI3.
  3. It's always some kind of _list object, and their API has a free method. A user should never use pure free. I think it's more of a problem that we're not consistent or even documenting everywhere when a user needs to handle memory
  1. How do we stay somewhat consistent with this. Any comment is a sort of documentation. Or do you propose // for in-line comments vs / ... / anything going into doxygen?
  2. Seems reasonable.
  3. Sounds like it should be reasonably easy to find the appropriate free method then and I suppose we'll improve documentation as we go then.
  1. /* / for function and struct declarations, doxygen groups (which I hope we won't need), and possibly on struct members. More or less interface documentation. // everywhere else. Up for discussion of course