Fix compilatition faliure for asynStatus in clang 17 #226
Conversation
|
❌ Build asyn 1.0.308 failed (commit df33718b41 by @tboegi) |
ericonr
left a comment
ericonr
left a comment
There was a problem hiding this comment.
There's a typo in the commit title, "compilatition" should be "compilation".
Do you think the commit message should include the whole compiler output? I think it could be resumed into a simpler and shorter text mentioning -Wenum-constexpr-conversion and the fact that asynPortDriver was extending an enum by defining additional constants (which is not possible in C++). The compiler version (clang-17) should also be enough.
The solution should mention that we are changing these "internal" enums into public ones, and address any (if there are) concerns over exposing that in the public interface. Is there another way to solve this? Should they be named something like asyn_internal__asynParamAlreadyExists or similar so that paramErrors.h can do #define asynParamAlreadyExists asyn_internal__asynParamAlreadyExists?
Finally, I don't think the commit should mention which files it touches; that information can be obtained with git log --name-status, after all.
| asynSuccess,asynTimeout,asynOverflow,asynError,asynDisconnected,asynDisabled | ||
| asynSuccess, | ||
| asynTimeout, | ||
| asynOverflow, | ||
| asynError, | ||
| asynDisconnected, | ||
| asynDisabled, | ||
| /* Extend asynManager error list . */ | ||
| asynParamAlreadyExists, | ||
| asynParamNotFound, | ||
| asynParamWrongType, | ||
| asynParamBadIndex, | ||
| asynParamUndefined, | ||
| asynParamInvalidList |
There was a problem hiding this comment.
Have you checked that this doesn't create any new warnings in any asynDriver code, since switch cases might not check for all these enums?
I think the comment is wrong, too. It should mention that the list is extended to cover enums used only by asynPortDriver code.
There was a problem hiding this comment.
Have you checked that this doesn't create any new warnings in any asynDriver code, since switch cases might not check for all these enums?
I see at least once switch which should be updated.
asyn/asyn/asynDriver/asynManager.c
Lines 3205 to 3216 in e2a281e
Glancing through the output of git grep switch, there are likely one or two others.
| #define asynParamAlreadyExists (asynStatus)(asynDisabled + 1) | ||
| #define asynParamNotFound (asynStatus)(asynDisabled + 2) | ||
| #define asynParamWrongType (asynStatus)(asynDisabled + 3) | ||
| #define asynParamBadIndex (asynStatus)(asynDisabled + 4) | ||
| #define asynParamUndefined (asynStatus)(asynDisabled + 5) | ||
| #define asynParamInvalidList (asynStatus)(asynDisabled + 6) |
There was a problem hiding this comment.
I dislike that this commit moves internal errors from asynPortDriver into the asynDriver header. But updating the asynPortDriver functions to return a different kind of error would probably be too complicated...
tboegi
force-pushed
the
fix-asynstatus-for-clang17
branch
from
244c3df to
38714cf
Compare
|
@ericonr @mdavidsaver Thanks for the reviews. A new patch should address your comments |
After installing a new clang under MacOS:
c++ --version
Apple clang version 17.0.0 (clang-1700.6.3.2)
Target: x86_64-apple-darwin24.6.0
Thread model: posix
InstalledDir: /Library/Developer/CommandLineTools/usr/bin
----------------
the compilation of a motor driver failed:
c++ -DUNIX -Ddarwin -O3 -g -Wall -arch x86_64 -fno-common -fPIC -I. -I../O.Common -I. -I. -I.. -I.../epics/modules/motor/include/compiler/clang -I.../epics/modules/motor/include/os/Darwin -I.../epics/modules/motor/include -I.../epics/modules/motor/include -I.../epics/base/../modules/asyn/include -I.../epics/base/include/compiler/clang -I.../epics/base/include/os/Darwin -I.../epics/base/include -c ../smarActMCS2MotorDriver.cpp
../smarActMCS2MotorDriver.cpp:147:10: error: integer value 8 is outside the valid range of
values [0, 7] for the enumeration type 'asynStatus' [-Wenum-constexpr-conversion]
147 | case asynParamWrongType:
| ^
[snip]
3 errors generated.
----------------
Problem:
Both asynManager and asynPortDriver are using the same enum
for different things:
asynManager uses the "basic" definitions asynSuccess..asynDisabled
and asynPortdriver needs asynParamAlreadyExists and friends for
the parameter interface.
However, all of the defintions are "public" and can be used in
derived classes.
Extendig an enum like done before is not allowed in C++, and the
clang developers chose to make it an error.
And probably other compilers will be more strict in the future as well.
Possible solutions:
a) One c-stylish way would be to define asynStatus as "int"
and use #defines for all the values.
b) Split asynStatus into 2 definitions:
asynStatusManager and asynStatusDriver
c) Define all values used in asyn at one place.
a) seems less attractive, since we would loose the ability to let
the compiler check enums.
b) Seems less attractive since it would break a lot of existing code
and force it to be changed to make sense.
Or add a lot of #ifndef EPICS_ASYN_SPLIT_STATUS legacey handling.
c) Seems to be the least invasive change.
Solution:
Define all possible values for asynStatus in asynDriver.h
Enhance asynManager::strStatus() to handle all new enum values.
tboegi
force-pushed
the
fix-asynstatus-for-clang17
branch
from
38714cf to
922d505
Compare
|
❌ Build asyn 1.0.309 failed (commit 997c69766b by @tboegi) |
|
❌ Build asyn 1.0.310 failed (commit 58d439393c by @tboegi) |
4 participants
After installing a new clang under MacOS:
the compilation of a motor driver failed:
Solution:
Define all possible values for asynStatus in asynDriver.h
Changes to be committed:
modified: asyn/asynDriver/asynDriver.h
modified: asyn/asynDriver/asynManager.c
modified: asyn/asynPortDriver/paramErrors.h