trilinos / Trilinos

Primary repository for the Trilinos Project
https://trilinos.org/
Other
1.2k stars 563 forks source link

Framework: Add "develop-to-master promotion version" macro #2338

Closed mhoemmen closed 3 years ago

mhoemmen commented 6 years ago

Add a macro somewhere in Trilinos, with an integer that gets incremented on each develop-to-master promotion.

@trilinos/framework

Current Behavior

I can't write code that distinguishes between different Trilinos develop-to-master promotions.

Motivation and Context

Last commit to Trilinos 12.0 was in 2015. Last minor version release (12.12) was in July 2017. This suggests that major version releases may never happen, and that develop-to-master promotions are now the de facto minor version releases. Thus, Trilinos major version numbers are no longer useful for tracking backwards compatibility issues. #2290 (where a harmless change announced literally 3 years ago may break applications that forward-declare templated classes) highlights the value of being able to distinguish between Trilinos develop-to-master promotions.

This issue hinders Tpetra FY18 goals. @kddevin @trilinos/tpetra

mhoemmen commented 6 years ago

@rrdrake and @jwillenbring expressed interest in this today.

barche commented 6 years ago

Interested too, after also hitting a compile error after #2290

mhoemmen commented 6 years ago

@barche

  1. FYI forward declarations of templated classes don't have a way to specify default template parameter values
  2. Please don't ever specify template parameters that have default values, unless you intend to use a nondefault value for them
barche commented 6 years ago

I hit this when mapping Tpetra types to Julia, specifically here: https://github.com/barche/Trilinos.jl/blob/master/deps/src/trilinoswrap/tpetra.cpp#L213 Here, T is e.g. a Tpetra::CrsGraph. I would have expected that the false would not need to be specified since it is the default, but if I don't put it explicitly the compiler says there are not enough template arguments.

FWIW, I'm glad this parameter is removed (it will simplify my code!), but in general I think it's a good idea to have a macro to detect breaking changes, so it's easier to support multiple versions. It's not really a pressing issue for me, since the Julia wrappers are still in heavy development and I can just use git master on all my machines.

jwillenbring commented 6 years ago

@trilinos/framework let’s discuss this at the Monday standup.

prwolfe commented 6 years ago

2 thoughts - shouldn't the code be embedded in a version - i.e. would not need to discriminate between versions, and what about git describe? If we are doing a single merge commit per promotion it will give us the count you are after. If we doing full history imports it won't.

mhoemmen commented 6 years ago

@barche wrote:

but if I don't put it explicitly the compiler says there are not enough template arguments.

Just curious -- what compiler are you using? I've only ever seen this when people do their own forward declarations of Tpetra classes.

I have some type aliases in the works that should make it easier to do backwards compatibility for Tpetra classes' template parameters.

barche commented 6 years ago

After realizing I forgot to remove the bool inthe template template parameter spec, the error I get on GCC 7.3.1 is:

expected a template of type ‘template<class, class, class> class T’, got ‘template<class LO, class GO, class N, bool isClassic> class Tpetra::CrsGraph’

Clang gives a similar error, it seems that default arguments are not taken into account when matching template template parameters.

mhoemmen commented 6 years ago

@barche Template parameters that are constexpr values (instead of types) are weird. They don't count as part of a parameter pack, for example. This is one of my motivations for getting rid of this ;-) .

github-actions[bot] commented 3 years ago

This issue has had no activity for 365 days and is marked for closure. It will be closed after an additional 30 days of inactivity. If you would like to keep this issue open please add a comment and/or remove the MARKED_FOR_CLOSURE label. If this issue should be kept open even with no activity beyond the time limits you can add the label DO_NOT_AUTOCLOSE. If it is ok for this issue to be closed, feel free to go ahead and close it. Please do not add any comments or change any labels or otherwise touch this issue unless your intention is to reset the inactivity counter for an additional year.

github-actions[bot] commented 3 years ago

This issue was closed due to inactivity for 395 days.