[Development] On the use of the inline keyword

[Ahmad Samir]

On 26/8/23 11:51, Volker Hilsheimer wrote:
> 
> 
>> On 25 Aug 2023, at 14:20, Ahmad Samir <a.samirh78 at gmail.com> wrote:
>>
>> On 25/8/23 14:11, Cristian Adam via Development wrote:
>>> The other way of fixing this is by using ... macros. The article at c++ - Importing inline functions in MinGW - Stack Overflow<https://stackoverflow.com/questions/11546403/importing-inline-functions-in-mingw>  mentions using inline in the class declaration.
>>> Their example works fine with both GCC MinGW and Clang MinGW. Visual C++ is also fine:
>>> #ifdef _WIN32
>>> #define Q_EXPORT_INLINE inline
>>> #else
>>> #define Q_EXPORT_INLINE
>>> #endif
>>> class __declspec(dllimport) MyClass {
>>> public:
>>>      Q_EXPORT_INLINE int myFunc2();
>>>      Q_EXPORT_INLINE int myFunc1();
>>> };
>>> inline int MyClass::myFunc2(void) {
>>>      return myFunc1();
>>> }
>>> inline int MyClass::myFunc1(void) {
>>>      return 0;
>>> }
>>
>> [...]
>>
>> The main deterrent to not fix it like the qstring.h patch[1] is the churn caused by changing many lines, in many files, right?
>>
>> But if affected code is going to be changed anyway to fix it with macros, it makes more sense to fix it like [1]; if you're changing those lines, may as well adhere to more standard guidelines: inline keyword is only used on function declaration and _not_ the definition.
>>
>>
>> My 2p's.
>>
>> [1] https://codereview.qt-project.org/c/qt/qtbase/+/498739
> 
> 
> Yes, if we have to change the code to fix the warnings, then the way Marc proposes, and how it has now been done for qstring.h, seems to be the way to go.
> 
> But yes, my concern is the work (I expect this can be easily automated with clazy, but still need to be reviewed), and the code churn that goes with it (the impact on `git blame`, and the inevitable conflicts when cherry-picking).
> 

Tentative clazy check: https://invent.kde.org/sdk/clazy/-/merge_requests/84

IIUC, the `git blame` issue can be mitigated by adding those commits to e.g. a 
.git-blame-ignore-revs file in each repo, see `git blame --ignore-revs-file` [1]. 
I saw something like that in one of Qt's repos, but can't remember which one... 
it's also used in almost all KDE's Frameworks repos.

Conflicts when cherry-picking could be a problem. But the changes can be limited 
to the bare minimum, i.e. only add inline keyword to declaration of affected 
methods (ignoring constexpr and function templates, both are implicitly inline IIUC).

(I'd rather the whole thing is done like qstring.h....).


> It seems to be a rare issue, triggered by specific circumstances. With the knowledge that we have now, we can fix issues when they arise, and don’t have to change all the problematic use right now.
> 
> That’s my understanding at least; I might be wrong, but we have been building Qt for a few decades on MinGW without this constantly blocking us.
> 

FWIW, that's my understanding too.

I've used that clazy check linked above (via clazy-standalone) on qtbase, it 
doesn't look too bad... if you ignore src/opengl, that dir alone:
$ git show --stat
26 files changed, 14857 insertions(+), 14857 deletions(-)

I've no idea about OpenGL, but it looks like that is generated code, so maybe `git 
blame` doesn't matter there that much.


[1] https://git-scm.com/docs/git-blame

Regards,
Ahmad Samir

-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 236 bytes
Desc: OpenPGP digital signature
URL: <http://lists.qt-project.org/pipermail/development/attachments/20230828/03f2f025/attachment.sig>