Conversation
... 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.
JamesCrook
left a comment
There was a problem hiding this comment.
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.
IGBC
left a comment
There was a problem hiding this comment.
Verified PR is clean of any code for purposes other than those described in the PR.
|
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 |
lllucius
left a comment
There was a problem hiding this comment.
Looks fine to me as well.
MSVC requires every symbol exported from a DLL to be explicitly declared or it won't be usable from linked code. Alternatively setting the |
@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. 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. |
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.
PROHIBITED is just a macro that expands to 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. |
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. |
|
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). |
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. |
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
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. |
|
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. |
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.