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

Move MAX, MIN and ABS to contiki-macros #883

Merged
merged 1 commit into from May 18, 2015

Conversation

cmorty
Copy link
Contributor

@cmorty cmorty commented Dec 1, 2014

Based on the discussion in #878 I started moving MIN, MAX and ABS macros to an extra file.

@adamdunkels
Copy link
Member

This is a good move, but please prefix the names with contiki_macros_, i.e. CONTIKI_MACROS_MAX().

But I'd suggest using a shorter name than contiki-macros.h.

@cmorty
Copy link
Contributor Author

cmorty commented Dec 1, 2014

Is it a good Idea to make these commonly used macros unnecessarily long? Especially as I can see no good reason to prefix them. The definitions are kind of a standard.
As for the filename, I don't really care. As there already is contiki-lib.h and contiki-net.h, etc

@adamdunkels
Copy link
Member

Lets put them in contiki.h and call them CONTIKI_MAX() and similar.

@cmorty
Copy link
Contributor Author

cmorty commented Dec 1, 2014

...I still don't see any advantage of using CONTIKI_MAX instead of just 'MAX'. (I can only repeat my arguments against it.)
As for the filename: There are quite some files that don't include "contiki.h", but use MIN/MAX. So maybe contiki-compiler.h is better, as these macros basically use the CPP to overcome shortcomings of the C-Language.

@adamdunkels
Copy link
Member

Good point, lets use the existing cc.h for this! https://github.com/contiki-os/contiki/blob/master/core/sys/cc.h

@cmorty
Copy link
Contributor Author

cmorty commented Dec 1, 2014

Ok, that seems reasonable. Will do.

@cmorty
Copy link
Contributor Author

cmorty commented Dec 1, 2014

Ok, fixed. While at it, I noticed a very inconsistent used of "" and <> when including files. This may lead to some very ugly side-effect. IMO we should used <> for all files from the contiki-tree and "" only for the files in the same folder. On the other side, using "" for everything would allow easy overrides of header files. (All in all I assume that very few codes actually know the difference. ;) )

@adamdunkels
Copy link
Member

The Contiki code uses "" for all Contiki headers and <> only for C library and underlying OS headers.

So it should be #include "sys/cc.h" and not #include <sys/cc.h>

@cmorty cmorty force-pushed the pull/global-macros branch 2 times, most recently from 3bda586 to 9abd760 Compare December 1, 2014 13:57
@nvt
Copy link
Member

nvt commented Dec 1, 2014

This is a welcome addition of macros, but the suggested macro that initiated this discussion is not included here: IS_POWER_OF_TWO. Perhaps we want this too, but I'm happy with the change as it is also. When this collection of macros may grow a bit later, however, it may be necessary to put it into another file than sys/cc.h, since that file seems to be more for handling different C compiler quirks.

@cmorty
Copy link
Contributor Author

cmorty commented Dec 1, 2014

@nvt My original idea was to introduce a new contiki-macros-h and then add IS_POWER_OF_TWO. But the is latter is not as easy to grep. ;) - But if I find some time I'll do that, too.

@nvt
Copy link
Member

nvt commented Jan 11, 2015

👍

@g-oikonomou
Copy link
Contributor

Where are we on this one? I mean it would naturally need rebased, but it'd be great to get an update (?)

@bthebaudeau
Copy link
Member

It would be great to merge this one now (after rebasing if needed), and to create a new PR later for #878.

@cmorty
Copy link
Contributor Author

cmorty commented May 11, 2015

Getting this merged rather soon would be nice. Had to fix two new files.

@bthebaudeau
Copy link
Member

Sorry, rebase needed.

@cmorty
Copy link
Contributor Author

cmorty commented May 15, 2015

There you go. Rebase seems fine. :)

@@ -39,7 +39,8 @@
#include <stdio.h>
#include <string.h>

#include "contiki.h"
#include <contiki.h>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to what @adamdunkels said, double quotes should be used here, not angle brackets.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That one slipped through. I'll look into it

@bthebaudeau
Copy link
Member

@adamdunkels: In the end, are you happy with the naming? You did not say if you finally agreed with @cmorty arguments. I vote for simplicity here: a prefix would bring no benefits.

@bthebaudeau
Copy link
Member

👍 from me.

@simonduq
Copy link
Member

and another 👍

@adamdunkels
Copy link
Member

👍

adamdunkels added a commit that referenced this pull request May 18, 2015
Move MAX, MIN and ABS to contiki-macros
@adamdunkels adamdunkels merged commit 730bda2 into contiki-os:master May 18, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants