Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace uses of SetMacro and GetMacro #43

Open
schuhschuh opened this issue Dec 17, 2015 · 6 comments
Open

Replace uses of SetMacro and GetMacro #43

schuhschuh opened this issue Dec 17, 2015 · 6 comments

Comments

@schuhschuh
Copy link
Member

The SetMacro and GetMacro definitions should be removed as they potentially conflict with other libraries. Instead, use irtkSetMacro and irtkGetMacro (or possibly even better, irtkPublicAttributeMacro to declare the attribute in the first place). These macros are defined in irtkObject.h.

@ghisvail
Copy link
Member

PR ?

@schuhschuh
Copy link
Member Author

I'll leave this on your plate. It's from legacy code. I added newer macros that can replace these and made use of them in all my new code.

@ghisvail
Copy link
Member

After further looking at the code, I have very mixed feelings about their usage to be honest. I'll explain why.

There is no denying that the design of these macros is borrowed from ITK / VTK and I can understand their usage there because of their complex object model. Our object model on the other hand is much simpler. My conclusion is that, as far as the content of these macros goes, their only purpose is to save a few keystrokes and mimic some sort of property concept. That I have no issue with.

I do have a problem however with their implementation for the following reason:

These macros introduce a deliberate syntactic error via a non-terminated declaration of a static void function with a carefully crafted name. The side-effect of this it to unnecessarily bloat the class body with dummy functions with no other purpose than triggering an hypothetical compilation error. I know this is meant to provide some sort of a hint from the compiler in case the macro is not used like a statement. But from a pure design perspective, this solution, albeit producing working code, sounds just wrong to me.

So my position is that, unless this design is reverted to something more conventional like what ITK / VTK is using, neither will I advocate usage of these macros publicly, nor will I replace existing code to use them.

@schuhschuh
Copy link
Member Author

You can just remove those static functions if you like, but AFAIK, they are not adding any binary code. The compiler will get rid of them. I don't see any issue with this. Also, I believe they are private and couldn't even be used by mistake.

EDIT: The being "private" is only for the irtkAttributeMacro type of macros (property style as you say), which anyway should be preferred. I rarely use the irtkGetMacro or irtkSetMacro (if ever).

Because of these macros, all you have to do is modify them to your liking, touching a single file in few places. If you are fine with the "extra semicolon" caused by not having such static dummy functions, just get rid of them.

@schuhschuh
Copy link
Member Author

deliberate syntactic error

How is that syntactically incorrect ? I don't get it. Wouldn't the compiler complain otherwise?

EDIT: The purpose is to disambiguate the usage of the macros. Otherwise, both of the following usages would be allowed:

irtkAttributeMacro(bool, Foo)
irtkAttributeMacro(bool, Foo);

And as you can even see from the syntax highlighting here, the first is indeed incorrect syntax unless the macro is expanded.

@ghisvail
Copy link
Member

Ok. I will use the opportunity of covering the whole source tree for fixing #7 and #8 to list the classes that could benefit from these macros and fix this subsequently. Thanks for the pointers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants