Bela: XenomaiLock review patches

[mossheim]

this PR revises XenomaiLock with some miscellaneous improvements:

• adhere to webkit naming conventions for functions (https://webkit.org/code-style-guidelines/#names)
• remove an undefined constructor declaration
• remove recurred boolean params from public interfaces
• improve type correctness (int vs bool)
• remove unused variables
• fix an incorrect printf format

i also redid the try_or_retry function to avoid std::function and have a bit cleaner interface that doesn't require passing actual names or object pointers explicitly. since the only purpose of those parameters is to debug by tracking events, using this instead of pointers to condition var or mutex should provide the same information, just with less code. same goes for using __func__ instead of handwritten function names -- the result is identical, but there is less chance of a mix-up due to copy-paste mistakes.

std::function should be avoided for performance-sensitive code because it may dynamically allocate if the stored callable is not a simple function pointer or reference wrapper; it's better to take the lambda as a parameter directly in this case, as it will very likely be inlined by the compiler that way. you can see an example of this here: https://godbolt.org/z/6zKh1a

[giuliomoro]

overall it looks good, thanks. There was an error in

    tryOrRetry([this, &lck]() { return __wrap_pthread_cond_wait(&this->cond, &lck.mutex()->mutex); }, enabled);

as the preprocessor was seeing 3 parameters being passed to the macro instead of 2 ... [no idea why]. I added parentheses around the first argument in 55da54e6a8b7489523e7b3ad41af14ee9d3de9e7.

Then I have two more suggestions is in 2dbb44c2364a6e098aef85f26ab8d1dc5f3a619c:

• in general, I prefer to use if (0 == a) instead of if (a == 0), as it prevents erroneous assignment in case one forgets one = (not my idea, I read this in some style guide). Again, clang would warn you of that, but gcc doesn't seem to. Feel free to accept this or not depending on how it fits with the overall style of Sc.
• I think in the last part of error handling in tryOrRetryImpl() the else if(!turnIntoCobaltThread()) misleading, in that it doesn't clearly show that that's actually an else { ... } :

    } else if (!turnIntoCobaltThread()) {
         // if we got EPERM, we are not a Xenomai thread
        xfprintf(stderr, "%s %p could not turn into cobalt\n", name, id);
        return false;
    }

Also, it was not easy to place the comment in the best place (which would be before the call to turnIntoCobaltThread(), so I reworked it a bit in a way that to me is clearer:

    } else {
         // if we got EPERM, we are not a Xenomai thread
        if (!turnIntoCobaltThread()) {
            xfprintf(stderr, "%s %p could not turn into cobalt\n", name, id);
            return false;
        }
     }

[mossheim]

the preprocessor was seeing 3 parameters being passed to the macro instead of 2 ... [no idea why].

ah, thanks for catching that -- the preprocessor doesn't care about most tokens, so when it sees the comma in the lambda capture list, it assumes those are two separate arguments -- [this and &lck]() { .... the only way to avoid it is to enclose in parens as you did.

in general, I prefer to use if (0 == a) instead of if (a == 0), as it prevents erroneous assignment in case one forgets one = (not my idea, I read this in some style guide). Again, clang would warn you of that, but gcc doesn't seem to. Feel free to accept this or not depending on how it fits with the overall style of Sc.

i actually really prefer this myself and agree with the reasoning for it. i'm just not very used to it. no complaints here!

I think in the last part of error handling in tryOrRetryImpl() the else if(!turnIntoCobaltThread()) misleading, in that it doesn't clearly show that that's actually an else { ... }

i am almost used to thinking of else { if (...) { ... } } as a typo because it can always be rewritten as else if(...) { ... }. i'm not sure if this is what you mean exactly, but i do find it a bit unclear that the first two conditionals are only checking the value of ret, while turnIntoCobaltThread() on the other hand does some actual work. plus you make a good point about the comment placement too.

i've added both your commits to this branch, thanks!

[giuliomoro]

it a bit unclear that the first two conditionals are only checking the value of ret, while turnIntoCobaltThread() on the other hand does some actual work.

yes that's exactly my point.