-
Notifications
You must be signed in to change notification settings - Fork 62
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update doc for adaptVerbose and mark it deprecated #447
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. This looks good. A few comments:
-
For the record, it looks like LLVM supports the GNU
deprecated
attributes but Intel does not:
Classic Intel: https://www.intel.com/content/www/us/en/docs/cpp-compiler/developer-guide-reference/2021-8/attributes.html
oneAPI Intel: https://www.intel.com/content/www/us/en/docs/dpcpp-cpp-compiler/developer-guide-reference/2024-2/attributes.html
LLVM: https://clang.llvm.org/docs/AttributeReference.html#deprecated -
Calling a deprecated function in the g++ build results in an warning (which our build promotes to an error):
https://github.com/SCOREC/core/actions/runs/10423443541/job/28870162471#step:5:587
A couple options come to mind:- Replace them with
adapt(...)
or some other variant that is acceptable (e.g., modified input parameters), or - Mark those locations so warnings are not emitted:
https://stackoverflow.com/a/3394268
I prefer the first as it leaves less work for us in the future.
Here are the locations that calladaptVerbose(...)
:
- Replace them with
~/develop/core (develop)$ git grep adaptVerbose
capstone_clis/capStoneAnisoAdapt.cc: ma::adaptVerbose(in, false);
capstone_clis/capStoneAnisoAdaptWing.cc: ma::adaptVerbose(in, false);
capstone_clis/capStoneAttachSolution.cc: ma::adaptVerbose(in, false);
capstone_clis/capStoneAttachSolution2.cc: ma::adaptVerbose(in);
capstone_clis/capStoneAttachSolutionStrandOnly.cc: ma::adaptVerbose(in, false);
capstone_clis/capStoneAttachSolutionUni.cc: ma::adaptVerbose(in, true);
capstone_clis/capStoneIsoAdaptB737.cc: ma::adaptVerbose(in, false);
ma/ma.cc:void adaptVerbose(Input* in, bool verbose)
ma/ma.cc:void adaptVerbose(const Input* in, bool verbose)
ma/ma.cc: adaptVerbose(makeAdvanced(in), verbose);
ma/ma.h:void adaptVerbose(Input* in, bool verbosef = false);
ma/ma.h:void adaptVerbose(const Input* in, bool verbosef = false);
python_wrappers/apf.i: void adaptVerbose(Input* in, bool verbosef = false);
test/capVol.cc: ma::adaptVerbose(in, false);
test/highOrderSizeFields.cc: ma::adaptVerbose(inAdv);
@joshia5 @cwsmith you may want to check if intel support the see: https://en.cppreference.com/w/cpp/language/attributes/deprecated If it doesn't you could define a |
Yeah, I like the idea of having the macro. Unfortunately, I don't see the 'deprecated' attribute (old or new C++14) listed in the oneAPI Intel compiler reference (linked above). |
C++ 17 guarantees
it may be worth checking if intel safely ignores |
We require C++11 but optionally enable C++14. Our local systems don't currently have access to the new Intel oneAPI compilers, but it looks like we may be able to download them: |
There are differences in adapt and adaptVerbose function as it is now (apart from printing out the mesh and other print statements) which include additional iteration(s) of refine+snap
core/ma/ma.cc
Lines 106 to 107 in 8959c59
and calling fixelementShapes Niter times
core/ma/ma.cc
Line 85 in 8959c59
This PR attempts to improve the function documentation in the header stating the above-mentioned differences and mark function deprecated for potential upcoming name-change (breaking)