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.

Sign up for GitHub

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

Jump to bottom

Rename internal data type enumeration from Type to DataType #2318

Merged
merged 2 commits into from
Jun 16, 2020
Merged

Rename internal data type enumeration from Type to DataType #2318

merged 2 commits into from
Jun 16, 2020

Conversation

bradking
Copy link
Collaborator

The enumeration added by #2310 is named Type, but DataType more precisely describes its purpose.

@bradking bradking requested a review from chuckatkins June 10, 2020 12:55
@bradking bradking mentioned this pull request Jun 10, 2020
Merged
bradking added 2 commits June 10, 2020 09:53
@bradking
utils: Avoid unnamed struct in adios_reorganize
f0bedea 
MSVC now warns:

    warning C5208: unnamed class used in typedef name cannot declare
    members other than non-static data members, member enumerations,
    or member classes
@bradking
Rename internal data type enumeration from Type to DataType
e326af1 
The enumeration added by commit 35d7abb (Convert internal data type
ids from std::string to an enumeration, 2020-06-04) is named `Type`, but
`DataType` more precisely describes its purpose.

Inspired-by: Kai Germaschewski <[email protected]>
@bradking bradking mentioned this pull request Jun 11, 2020
Merged
@bradking
Copy link
Collaborator Author

CI is happy here except for timeouts on macos macos1015_xcode111_make which occurred repeatedly.

@bradking
Copy link
Collaborator Author

I've opened #2322 specifically to fix the new MSVC warning with no other changes. Once that is merged this can be rebased.

@chuckatkins chuckatkins merged commit 0288859 into ornladios:master Jun 16, 2020
@bradking bradking deleted the data-type branch June 16, 2020 15:59
@ax3l
Copy link
Contributor

ax3l commented Jun 18, 2020 •
edited
Loading

I saw that merging this PR two days ago broke our nightly integration checks compiling against ADIOS2's development branch. We use adios2::GetType<...>, which we thought is public API and is now renamed, what do you recommend? :)

(We happily adopt/alias etc. for a future release, no reason for us not to change it to something more expressive.)

Refs.:

cc @franzpoeschel

@bradking bradking mentioned this pull request Jun 18, 2020
Merged
@bradking
Copy link
Collaborator Author

@ax3l thanks. I had not intended to change the public-facing name. I've reverted that part in #2342.

@pnorbert
Copy link
Contributor

Thanks. It broke E3SM's adios driver too. There are functions there to map between pnetcdf and adios2 types.

However, in the future it may be useful for exposing the DataType publicly as well. It would make coding cleaner in my opinion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chuckatkins chuckatkins Awaiting requested review from chuckatkins

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bradking @ax3l @pnorbert @chuckatkins