Skip to content

Comments

Linkage#856

Merged
Paul-Licameli merged 3 commits intoaudacity:masterfrom
Paul-Licameli:linkage
May 10, 2021
Merged

Linkage#856
Paul-Licameli merged 3 commits intoaudacity:masterfrom
Paul-Licameli:linkage

Conversation

@Paul-Licameli
Copy link
Collaborator

Prepare many things to keep linking on Windows after the executable gets split up.

Another preliminary chore , but also a sweep touching many lines in the second commit.

Please review the first commit, and glance over the second and confirm I'm only adding AUDACITY_DLL_API in many places.

... Which will be needed for various reasons for Windows builds of certain
modularizations, which will otherwise complain that they can no longer
generate them as inlines.

In one case, deleted copies require explicitly defaulted moves, but they will
work as generated inline.
... in many places where the function call will later need to be between
modules (or libraries, or the executable) and the annotation will be a necessity
to keep the linkage working on Windows.

That's all that this sweeping commit does.
Copy link
Contributor

@JamesCrook JamesCrook left a comment

Choose a reason for hiding this comment

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

I approve this pull request.

To table for later, some discussion of using different macros to document which interfaces are at least heading in the direction of becoming stable reusable APIs and which ones are highly in flux still.

I think it is really important to now document in the wiki (or in a doxygen topic document so it stays with the code) the bigger reasoning for this change, the use of PROHIBITED, the requirements on compiler/C++ version. Perhaps some documentation too of where interfaces are beginning to stabilise? This change needs to be understood in context.

PROHIBITED feels (based on my coding style) like extra inconvenience in development. However, if locking more and more things down for safety becomes ubiquitous, then PROHIBITED is just idiom, part and parcel of developing in Audacity - something every Audacity developer must learn once. So I have no problem with it, provided we are heading towards systematic safer development with well documented rationale. The danger I see is in (a) transition, having a higgledy piggledy mix of what is done safely and what isn't that is harder to work with (b) rationale only known/understood by the developer (you, Paul) who is making the code safer.

Copy link

@IGBC IGBC left a comment

Choose a reason for hiding this comment

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

Verified PR is clean of any code for purposes other than those described in the PR.

@IGBC
Copy link

IGBC commented May 10, 2021

Out of curiousity could someone explain how the AUDACITY_DLL_API decorator on structs and functions effects linking? I am new to how linking on windows works

Copy link
Contributor

@lllucius lllucius left a comment

Choose a reason for hiding this comment

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

Looks fine to me as well.

@Be-ing
Copy link

Be-ing commented May 10, 2021

Out of curiousity could someone explain how the AUDACITY_DLL_API decorator on structs and functions effects linking? I am new to how linking on windows works

MSVC requires every symbol exported from a DLL to be explicitly declared or it won't be usable from linked code. Alternatively setting the WINDOWS_EXPORT_ALL_SYMBOLS property on the CMake target can take care of that automatically.

@Paul-Licameli
Copy link
Collaborator Author

Paul-Licameli commented May 10, 2021

Verified PR is clean of any code for purposes other than those described in the PR.

@IGBC, I am warning you once that I will treat all of your comments on my pull requests as unwelcome spam, and summarily delete them, if after 24 hours, I do not see an apology to my colleague Steve Daulton for the bullying tone you took in this comment.

#699 (comment)

If you want to be a serious participant in the conversation about Audacity, approach the established team members with more humility and respect.

As all serious participants should treat each other.

@Paul-Licameli Paul-Licameli merged commit 406b23c into audacity:master May 10, 2021
@Paul-Licameli
Copy link
Collaborator Author

I approve this pull request.

To table for later, some discussion of using different macros to document which interfaces are at least heading in the direction of becoming stable reusable APIs and which ones are highly in flux still.

Stability and good documentation of the interfaces for modules may be a task for later. My exploratory restructurings accomplished as a side effect the identification of the classes and functions that must become part of the interfaces (at least those that are not inline) because MSVC requires this annotation so that things will link.

So the scan for the _API macros will help separate what needs more careful documentation from what is implementation detail.

When a module is separated, AUDACITY_DLL_API will have to be searched-and-replaced with some other macro appropriate to the new module, like MOD_LABEL_TRACK_API. See commit 9fb3252 where I caused CMake to generate the definitions of those macros.

I think it is really important to now document in the wiki (or in a doxygen topic document so it stays with the code) the bigger reasoning for this change, the use of PROHIBITED, the requirements on compiler/C++ version. Perhaps some documentation too of where interfaces are beginning to stabilise? This change needs to be understood in context.

PROHIBITED feels (based on my coding style) like extra inconvenience in development. However, if locking more and more things down for safety becomes ubiquitous, then PROHIBITED is just idiom, part and parcel of developing in Audacity - something every Audacity developer must learn once. So I have no problem with it, provided we are heading towards systematic safer development with well documented rationale. The danger I see is in (a) transition, having a higgledy piggledy mix of what is done safely and what isn't that is harder to work with (b) rationale only known/understood by the developer (you, Paul) who is making the code safer.

PROHIBITED is just a macro that expands to
= delete
Which since C++11 causes a compilation error when overload resolution chooses the function declared with it. It's the proper way now to guarantee that a class is no-copy by making such declarations of copy construction and assignment.

Why do I use the macro? It began with that naked-new cleanup years ago. I wanted an easy textual scan for all uses of the delete keyword, to verify there are not naked deletes that should be fixed with smart pointers and RAII idiom. This macro just keeps those uses of = delete out of the textual scan.

It's not really about "safety" unless that term includes non-leakage of memory.

@lllucius
Copy link
Contributor

Verified PR is clean of any code for purposes other than those described in the PR.

@IGBC, I am warning you once that I will treat all of your comments on my pull requests as unwelcome spam, and summarily delete them, if after 24 hours, I do not see an apology to my colleague Steve Daulton for the bullying tone you took in this comment.

Oh, you didn't know Paul? Lauren is a member of the open source S.W.A.T. team and is our new PR auditor. She takes her commitment to Audacity VERY seriously too since she's been a (self appointed) "member" for all of a few days. I hope she can commit the years upon years that the rest of the team has put into the project.

@Be-ing
Copy link

Be-ing commented May 10, 2021

By all means @IGBC, keep an eye on pull requests being opened. But sarcastic code reviews and comments don't help anything (that goes for both sides).

@Paul-Licameli
Copy link
Collaborator Author

Verified PR is clean of any code for purposes other than those described in the PR.

@IGBC, I am warning you once that I will treat all of your comments on my pull requests as unwelcome spam, and summarily delete them, if after 24 hours, I do not see an apology to my colleague Steve Daulton for the bullying tone you took in this comment.

Oh, you didn't know Paul? Lauren is a member of the open source S.W.A.T. team and is our new PR auditor. She takes her commitment to Audacity VERY seriously too since she's been a (self appointed) "member" for all of a few days. I hope she can commit the years upon years that the rest of the team has put into the project.

I share your sense of frustration. But I will also allow those who know something about code (and this one might), and who want to offer serious suggestions (rather than presuming to dictate to us how we must do things), a chance to change their tone. So I'm withholding my own sarcasm.

I don't want an epic argument about etiquette starting here either. So please, enough said from us, unless @IGBC has an answer.

The pull request, meanwhile, is merged (by me) after your examination of it.

@IGBC
Copy link

IGBC commented May 10, 2021

Verified PR is clean of any code for purposes other than those described in the PR.

@IGBC, I am warning you once that I will treat all of your comments on my pull requests as unwelcome spam, and summarily delete them, if after 24 hours, I do not see an apology to my colleague Steve Daulton for the bullying tone you took in this comment.

#699 (comment)

If you want to be a serious participant in the conversation about Audacity, approach the established team members with more humility and respect.

As all serious participants should treat each other.

Apologies to @SteveDaulton for the tone I took with him in #699 (comment). Full disclosure I was actually drunk when I posted that. I consider it a lesson learnt I probably shouldn't log into GitHub at all when not in a professional mindset. I wish to also extend my apologies to anyone else who felt attacked or unwelcome because of my conduct in #699

Verified PR is clean of any code for purposes other than those described in the PR.

@IGBC, I am warning you once that I will treat all of your comments on my pull requests as unwelcome spam, and summarily delete them, if after 24 hours, I do not see an apology to my colleague Steve Daulton for the bullying tone you took in this comment.

Oh, you didn't know Paul? Lauren is a member of the open source S.W.A.T. team and is our new PR auditor. She takes her commitment to Audacity VERY seriously too since she's been a (self appointed) "member" for all of a few days. I hope she can commit the years upon years that the rest of the team has put into the project.

Well ouch. But mostly not untrue. In #835 I did point out that I would be monitoring all commits from now on. And I promise you I will not be going away in a few days either. I don't consider it some sort of open source SWAT manouvour either. I think its an important roll in the foss community.

As for my remark in my code audit. It is not intended sarcastically or with malice, only as a notice to anyone who looks at this PR and sees 1000 lines changed that they have been checked and that we don't need a repeat of #835. Nobody (especially me) wants all that drama to happen again.

So in summary. apologies to @SteveDaulton and @Paul-Licameli. I am here to help. Message received, I will take a more relaxed tone with the community from here on out.

Back on topic I am pleased an exited to see work commencing on extracting some of the dependencies out of the binary, and if anyone is interested in handing out work packets I am more than happy to go do some boilerplate work.

@Paul-Licameli
Copy link
Collaborator Author

I accept your apology, on my own behalf. I won't speak for Steve.

As for watching the commits, you may have noticed a big line count in this one. True. But as you see, it was one of those boring routine sweeps of things. Nothing like the inclusion of a whole new library.

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.

5 participants