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

Feature/stringid #5

Merged
merged 10 commits into from
Apr 27, 2016
Merged

Feature/stringid #5

merged 10 commits into from
Apr 27, 2016

Conversation

whaeck
Copy link
Member

@whaeck whaeck commented Apr 26, 2016

Added a template identifier class with internal string validation: utility::Id. The underlying id type is a string.

This Id class could be implemented as an interface from which different identifiers can inherit. The problem with this approach is that identifiers based on such an interface can be interchanged using the Id base class. E.g. an identifier for a Car could be used as if it were the identifier of a Planet if the base class is used.

By associating a template to this class (to indicate which object is identified by the Id), it will not be possible to confound Id types with different template types (because they will be considered as different types). For example: utility::Id and utility::Id are thus different types and cannot be interchanged.

The Id class also calls a template function to validate the string used in the identifier. By implementing a specialisation of the validateId function for a given type T, we can reject invalid identifiers at construction.

* types). For example: utility::Id<Car> and utility::Id<Planet> are thus
* different types and cannot be interchanged.
*/
template <typename T> class Id final {
Copy link
Member

Choose a reason for hiding this comment

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

The name of the class should be ID. It goes against the strict naming rules, but it is an abbreviation, so it should be all caps.

@jlconlin
Copy link
Member

Overall looks pretty good. There needs to be a slight change in the name of the class, but that's not a big deal.

I'm curious what @apmccartney thinks about this class. I can't see right away an immediate need for the class, but I can also see how it could be useful. Please educate me about this.

@whaeck
Copy link
Member Author

whaeck commented Apr 26, 2016

@jlconlin This class will allow us to easily create identifier objects with underlying validation. The interface for any string based identifier is actually the same for each one. Using an interface for this would be a good thing but the problem is that if you store the base class, you'll be able to compare identifiers of objects that have nothing to do with one another (hence the example of Planet and Car). By adding the template, the identifier becomes a different type for each object we wish to identify.

In addition, we can add a validateId function to make sure that an Id object is a valid object for the type it is identifying.

For example:

// forward declaration of the Reaction class
nuclearData::API::Reaction {};

// typedef the ReactionId
typedef utility::Id<nuclearData:API::Reaction> ReactionId;

// define the validateId function
template <> bool utility::validateId<nuclearData::API::Reaction>(const string& id) noexcept {

  if (id == "total") {

    return true;
  }
  return false;
}

@whaeck whaeck closed this Apr 26, 2016
@whaeck whaeck reopened this Apr 26, 2016
/** @}
*/

/** @{
Copy link
Member

Choose a reason for hiding this comment

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

@jlconlin doxygen has grouping specifications apparently.

Copy link
Member

Choose a reason for hiding this comment

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

What do you mean?

Copy link
Member

Choose a reason for hiding this comment

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

@@ -0,0 +1 @@
setupTest("Id")
Copy link
Member

Choose a reason for hiding this comment

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

Missed name update.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice catch!

@apmccartney apmccartney merged commit 7e62603 into master Apr 27, 2016
apmccartney added a commit that referenced this pull request Apr 14, 2017
a3e5adb constrain configuration types
470c254 directory installation correction
48ccecc regex error
44886a1 spacing in linker flags
cc89730 bugfix
6457dca rounding math removed and coverage extension
6c360a0 correction
35a3091 coverage in tests necessary for header-only libraries
edd43bb local changes
cbd8068 apple clang
8fe650a clang compliant debug flags
06fc1dc config
e6fb36d more updates
982fa8b update
b07c58c split
d840594 directory installation
5a03ea0 whitespace
4103d38 dependencies
75d0f1d subproject linking bug
5e9ac83 missung header extension
2657253 ignore path default
c486ab6 only verify version if version is available
9507100 bug
65038bd wrong order
cd179c1 bug
2cb9403 precedence
d9b76f1 bug in degenerate case
e86e3fd need more for subproject
5faa5b8 another bug
2493440 circular
d854e3e bug
50877ee correction
d6bac61 underscore keys
f93301c tweaks
2ff8fc0 external projects are hard
5bd040d Merge pull request #8 from njoy/refactor/generalize
ebb26a2 consider subprojects
0fd9a10 Merge pull request #6 from njoy/refactor/generalize
0d9c813 manual merge
22daf46 Merge pull request #5 from njoy/refactor/generalize
d19af43 njoy2016 testing/debugging
1527f74 debugged using njoy
64aca82 refactor
dce72af Merge pull request #3 from njoy/feature/collect_subprojects
d51b28a long lines
3164c69 don't build subproject executables
bae56cd correction
c10fdbd Correctly handle multiple test resources
f7c0ece offline script
e4b5ce8 better fortran module organization
fbd3876 removed project specific files
aad4441 testing work

git-subtree-dir: metaconfigure
git-subtree-split: a3e5adbd5c597d18898df0c63d2aeefa0dc3c230
@jlconlin jlconlin deleted the feature/stringid branch July 24, 2018 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants