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

constexpr helpers to identify core version #5269

Merged
merged 10 commits into from
Nov 29, 2018
Merged

Conversation

d-a-v
Copy link
Collaborator

@d-a-v d-a-v commented Oct 22, 2018

No description provided.


#define CORE_ESP8266_MAJOR (2)
#define CORE_ESP8266_MINOR (4)
#define CORE_ESP8266_REVISION (2)
Copy link
Member

Choose a reason for hiding this comment

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

There's partial overlap between these and the ones in core_version.h.

Can defines in core_version.h be written using these new ones?

package/README.md Outdated Show resolved Hide resolved
@d-a-v d-a-v mentioned this pull request Oct 23, 2018
6 tasks
@earlephilhower
Copy link
Collaborator

I really need to look at this closer, but the one thing I see is that as-defined, development/RC versions will have a greater numeric value than the release version.

@d-a-v
Copy link
Collaborator Author

d-a-v commented Oct 25, 2018

I really need to look at this closer, but the one thing I see is that as-defined, development/RC versions will have a greater numeric value than the release version.

I fully agree with saying that we should have 2.4.1 < 2.4.2 < 2.5.0rc1 < 2.5.0rc2 < 2.5.0.
The "encoding" way I proposed would give 24210 for 2.5.0rc1.
24210 is encoding either 2.5.0rc1 or 2.4.3rc1 (depending on what's after 2.4.2).
Encoding is major.minor.revision.candidate-before-next-release.devel.
devel should increase with API changes (signature update, or addition, or removal).
That's not intuitive, but the order is kept.
Any proposal for better, ordered, and intuitive encoding is welcome.

// CORE_ESP8266_VERSION:
// 2.4.3-dev  (after 2.4.2 release), or            24201
// 2.5.0-dev  (after 2.4.2 release)                24201
// 2.5.0-rc1  first candidate before next release  24210
// 2.5.0-rc1+ dev after first candidate            24211
// 2.5.0-rc2  second candidate                     24220
// 2.5.0      release                              25000
// 2.5.1-dev                                       25001
// 2.5.1-rc1                                       25010
// 2.5.1      release                              25100

package/README.md Outdated Show resolved Hide resolved
@devyte
Copy link
Collaborator

devyte commented Oct 26, 2018

Some comments:

  • The -rcN has caused a whole lot of confusion to users, who think that the -rc release is newer than the final release. The last time I saw that was only a few days ago "updating to something-rc" 🤦‍♂️
    I suggest changing to -bN or -betaN which I think would make it more evident that it's not the final release.
  • Something cool, but not necessarily useful for the use case here:
    The version comparison can be implemented as a constexpr function, which would resolve at compile-time into a const value.
    Here is a reference I think has a useful explanation.

Quick untested example put together on cpp.sh:

#include <iostream>

#define MAJOR 2
#define MINOR 4
#define SUB 3
#define BETA 1

constexpr bool
coreVersionLess(const int major, const int minor, const int sub, const int beta)
{
    return (major < MAJOR || (major == MAJOR && 
                                (minor < MINOR || (minor == MINOR && 
                                                    (sub < SUB || (sub == SUB && 
                                                                    ( (BETA > 0 && beta == 0) || (BETA > 0 && beta > 0 && BETA < beta) )
                                                                  )
                                                    )
                                                  )
                                )    
                             )
            );
}

int main()
{
    constexpr bool ret1 = coreVersionLess(2,4,2,0);
    constexpr const char * res1 = ret1 ? "older" : "newer";
    std::cout << res1 << std::endl;
    
    constexpr bool ret2 = coreVersionLess(2,4,5,0);
    constexpr const char * res2 = ret2 ? "older" : "newer";
    std::cout << res2 << std::endl;
}

I said not necessarily useful, because this evaluates at compile time, not at preprocessor time, so you can't use it to conditionally compile code. I mention it only to raise awareness and to keep it in mind when looking for ways to (slightly) reduce bin size.

  • Having said that, you can do something equivalent with preprocessor macros, which then does allow you to conditionally compile code:
#include <iostream>

#define MAJOR 2
#define MINOR 4
#define SUB 3
#define BETA 1

#define coreVersionLess(major, minor, sub, beta) \
(\
    (major < MAJOR || (major == MAJOR && \
                        (minor < MINOR || (minor == MINOR && \
                                            (sub < SUB || (sub == SUB && \
                                                            ( (BETA > 0 && beta == 0) || (BETA > 0 && beta > 0 && BETA < beta) ) \
                                                          ) \
                                            ) \
                                          ) \
                        ) \
                      ) \
    ) \
) 

int main()
{
    const bool ret1 = coreVersionLess(2,4,2,0);
    const char * res1 = ret1 ? "older" : "newer";
    std::cout << res1 << std::endl;
    
    const bool ret2 = coreVersionLess(2,4,5,0);
    const char * res2 = ret2 ? "older" : "newer";
    std::cout << res2 << std::endl;
    
    #if coreVersionLess(2,4,2,0)
        std::cout << "cond1" << std::endl;   
    #endif
    
    #if(coreVersionLess(2,4,5,0))
        std::cout << "cond2" << std::endl;    oh-noThisIsASyntaxErrorThatDoesntLetYouCompileButTheConditionalSkipsIt!!
    #endif
}
  • Of course, other comparison functions can be implemented: LessEq, Greater, GreaterEq, etc..
  • If the user needs something beyond the standard comparator logic, then he can follow the same logic and implement his own function.

@d-a-v
Copy link
Collaborator Author

d-a-v commented Oct 27, 2018

Using constexpr and git describe result, processed during compilation:

SDK:3.0.0-dev(c0f7b44)/Core:v242094-git:2.4.2-94-gdb7d4472/lwIP:STABLE-2_1_0_RELEASE/glue:arduino-2.4.2-13-g80224f0/BearSSL:f55a6ad

edit: numeric version is not added to ESP.getFullVersion(), and it reads now as 20402094

@d-a-v d-a-v changed the title Allowing API changes: add helper defines to identify versions constexpr helpers to identify core version Oct 27, 2018
Copy link
Collaborator

@earlephilhower earlephilhower left a comment

Choose a reason for hiding this comment

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

It's like magic. I hope it never breaks, because it'll take Stroustrop himself to fix it. 👍

Copy link
Collaborator

@devyte devyte left a comment

Choose a reason for hiding this comment

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

This is pretty good: constexpr is certainly used as I explained, and the logic is reduced in the assembly to reading a const value at compile time (I.e.: a good example of no-overhead programing).
However, and I mention this just to have it explicit somewhere, the user can't do conditional compile of code in the same way as with preprocessor macros. It should be possible to conditionally compile code using this in conjunction with SFINAE, but usage becomes more complex. We can explore that later, though.
Some comments about style:

  • instead of having a bunch of functions with prefix "esp8266", it would be better to remove the prefix and put the functions in namespace "esp8266".
  • generic functions, such as the constexpr atoi algorithm, should be factored out for possible reuse. They could be put in a subnamespace within esp8266, e.g. const_str or conststrlib or whatever. I'll leave that decision to @d-a-v :)

@d-a-v d-a-v merged commit c677714 into esp8266:master Nov 29, 2018
@d-a-v d-a-v deleted the versiondef branch November 29, 2018 16:10
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

Successfully merging this pull request may close these issues.

4 participants