Skip to content

Conversation

@naughtont3
Copy link
Contributor

Signed-off-by: Thomas Naughton naughtont@ornl.gov

Signed-off-by: Thomas Naughton <naughtont@ornl.gov>
@naughtont3
Copy link
Contributor Author

@jjhursey @jsquyres I started to jot this down during meeting today and then figured I'd go ahead and just try adding the paragraph. Much room for improvement, but maybe provides a spot for other ticket to document the current "expectations" for use of DECLSPEC.

Signed-off-by: Thomas Naughton <naughtont@ornl.gov>
naughtont3 and others added 2 commits September 6, 2022 17:59
Co-authored-by: Jeff Squyres <jsquyres@users.noreply.github.com>
Signed-off-by: Thomas Naughton <naughtont@ornl.gov>
@naughtont3
Copy link
Contributor Author

@jsquyres Thanks for feedback. BTW, was the delineation for the DECLSPEC/MODULE_DECLSPEC accurate as to your understanding? Just want to double check myself.

Copy link
Member

@jsquyres jsquyres left a comment

Choose a reason for hiding this comment

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

@naughtont3 Do you want to add this content to the docs/developer/source-code.rst file in #10772?


The ``*_DECLSPEC`` macros provide a method to annotate symbols to indicate
their intended visibility when compiling dynamically shared object files
(e.g., `libmpi.so`).
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
(e.g., `libmpi.so`).
(e.g., ``libmpi.so``).
Comment on lines 18 to 19
The ``*_DECLSPEC`` attribute is used to declare that the developer wants a
symbol to be usable outside of that library/DSO. For example,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The ``*_DECLSPEC`` attribute is used to declare that the developer wants a
symbol to be usable outside of that library/DSO. For example,
The ``*_DECLSPEC`` attributes are used to declare that a
symbol is to be visible outside of that library/DSO's scope. For example,

* ``*_DECLSPEC``: controls visibility on individual functions/data structures
* ``*_MODULE_DECLSPEC``: controls visibility of MCA module (component) structure

Copy link
Member

Choose a reason for hiding this comment

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

Nit: extra blank line at the end of the file.

Comment on lines 28 to 29
* ``*_DECLSPEC``: controls visibility on individual functions/data structures
* ``*_MODULE_DECLSPEC``: controls visibility of MCA module (component) structure
Copy link
Member

Choose a reason for hiding this comment

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

Looking at the code today, I wonder if the MODULE_DECLSPEC is left over from when we supported Windows. Today, DECLSPEC and MODULE_DECLSPEC resolve to the same back-end compiler attribute. I have a super-dim recollection that they might have been different on Windows, and we therefore needed 2 different macros.

But since we've long-since stopped supporting Windows, should we ditch the MODULE_DECLSPEC macro?

Copy link
Member

Choose a reason for hiding this comment

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

👍 for removing MODULE_DECLSPEC if it is redundant.

Copy link
Member

@jsquyres jsquyres Sep 7, 2022

Choose a reason for hiding this comment

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

I just checked Open MPI v1.0.0, and I confirm: in Windows, there was a difference between exporting the component symbol and exporting other symbols.

Given that we don't support native Windows builds any more, I think we should remove the distinction.

@naughtont3 I can make a PR for the code change if you want to update the text.

Copy link
Member

Choose a reason for hiding this comment

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

PR to remove MODULE_DECLSPEC: #10774

Copy link
Member

@jjhursey jjhursey left a comment

Choose a reason for hiding this comment

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

The text looks good to me. Thanks for writing this up!

@naughtont3
Copy link
Contributor Author

@naughtont3 Do you want to add this content to the docs/developer/source-code.rst file in #10772?

@jsquyres Yes, it seems like that is better spot. I'll roll rest of your feedback into this PR to include the elimination of MODULE_DECLSPEC bits. Do you just want me to open a PR on #10772?

Signed-off-by: Thomas Naughton <naughtont@ornl.gov>
@naughtont3
Copy link
Contributor Author

@jsquyres I create jsquyres#11 and we can just close this item once things are rolled into #10722.

@jsquyres
Copy link
Member

jsquyres commented Sep 8, 2022

This PR has been made moot by #10780

@jsquyres jsquyres closed this Sep 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

3 participants