Skip to content

[core/cling] Replace deprecated calls and enable warning #17666

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

Merged
merged 5 commits into from
Feb 11, 2025

Conversation

hahnjo
Copy link
Member

@hahnjo hahnjo commented Feb 7, 2025

No description provided.

@hahnjo hahnjo added the in:Cling label Feb 7, 2025
@hahnjo hahnjo requested a review from devajithvs February 7, 2025 10:13
@hahnjo hahnjo self-assigned this Feb 7, 2025
Copy link
Member

@vgvassilev vgvassilev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@hahnjo
Copy link
Member Author

hahnjo commented Feb 7, 2025

There are some more warnings with libc++:

  Warning: /Users/sftnight/ROOT-CI/src/interpreter/cling/lib/Interpreter/ValuePrinter.cpp:475:31: warning: 'codecvt_utf8_utf16<char16_t>' is deprecated [-Wdeprecated-declarations]
      std::wstring_convert<std::codecvt_utf8_utf16<value_type>, value_type> Convert;
                                ^
  /Users/sftnight/ROOT-CI/src/interpreter/cling/lib/Interpreter/ValuePrinter.cpp:503:38: note: in instantiation of function template specialization 'cling::encodeUTF8<char16_t>' requested here
      return utf8Value(Str, N, Prefix, encodeUTF8);
                                       ^
  /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/c++/v1/codecvt:541:28: note: 'codecvt_utf8_utf16<char16_t>' has been explicitly marked deprecated here
  class _LIBCPP_TEMPLATE_VIS _LIBCPP_DEPRECATED_IN_CXX17 codecvt_utf8_utf16
                             ^
  /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/c++/v1/__config:862:41: note: expanded from macro '_LIBCPP_DEPRECATED_IN_CXX17'
  #    define _LIBCPP_DEPRECATED_IN_CXX17 _LIBCPP_DEPRECATED
                                          ^
  /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/c++/v1/__config:835:49: note: expanded from macro '_LIBCPP_DEPRECATED'
  #      define _LIBCPP_DEPRECATED __attribute__((deprecated))
                                                  ^

I need to investigate...

@hahnjo hahnjo marked this pull request as draft February 7, 2025 12:11
Copy link

github-actions bot commented Feb 8, 2025

Test Results

    18 files      18 suites   4d 5h 13m 51s ⏱️
 2 715 tests  2 714 ✅ 0 💤 1 ❌
47 172 runs  47 171 ✅ 0 💤 1 ❌

For more details on these failures, see this check.

Results for commit 61e696e.

♻️ This comment has been updated with latest results.

FileEntry::getName() is deprecated and the code should use
FileEntryRef::getName() instead.
The header and all declarations are deprecated since C++17 and will
be removed with C++26. Alywas use the LLVM conversion routines which
were already used in the past for older compilers.
@hahnjo hahnjo marked this pull request as ready for review February 10, 2025 07:40
@hahnjo
Copy link
Member Author

hahnjo commented Feb 10, 2025

There are some more warnings with libc++:

Ok, we can get away just removing the code using <codecvt> and using the LLVM implementation. Unless there are immediate objections to that, I will merge later today / tomorrow morning.

@ferdymercury
Copy link
Contributor

ferdymercury commented Feb 10, 2025

Ok, we can get away just removing the code using <codecvt> and using the LLVM implementation

Thanks!
Looking at https://github.com/search?q=repo%3Aroot-project%2Froot%20codecvt&type=code
there are a couple of places more using things like # if __GNUC__ > 4 && __has_include("codecvt") that could be simplified.
(maybe for another PR @guitargeek ?)

@hahnjo
Copy link
Member Author

hahnjo commented Feb 10, 2025

Looking at https://github.com/search?q=repo%3Aroot-project%2Froot%20codecvt&type=code there are a couple of places more using things like # if __GNUC__ > 4 && __has_include("codecvt") that could be simplified. (maybe for another PR?)

I only see exactly one place, in bindings/pyroot/cppyy/CPyCppyy/src/Converters.cxx, which needs to be fixed upstream first I think (edit: and is only for Python 2, which ROOT doesn't support anymore). Which "couple of places" do you see? But in any case, this is out of the scope of this PR which is only about Cling and Cling-including parts of core/.

@ferdymercury
Copy link
Contributor

ferdymercury commented Feb 10, 2025

I see. So maybe then:

grep -n -r -i codecvt *
cmake/modules/SetUpWindows.cmake:69:      set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -D_SILENCE_CXX17_ITERATOR_BASE_CLASS_DEPRECATION_WARNING -D_SILENCE_CXX17_CODECVT_HEADER_DEPRECATION_WARNING")
interpreter/cling/CMakeLists.txt:241:      set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -D_SILENCE_CXX17_CODECVT_HEADER_DEPRECATION_WARNING")
interpreter/cling/include/cling/std_darwin.modulemap:40:module std_codecvt [system] {

@hahnjo
Copy link
Member Author

hahnjo commented Feb 10, 2025

@ferdymercury I'm sorry, I totally don't get what you are saying. Where do you run that command, for me it looks very much different. And why would we want to add _SILENCE_CXX17_CODECVT_HEADER_DEPRECATION_WARNING? I argue that we can perfectly live (in Cling) with the implementation using LLVM's conversion functions. Other uses of <codecvt> are not subject of this PR and I don't want to spend my time on them here.

@ferdymercury
Copy link
Contributor

ferdymercury commented Feb 10, 2025

@ferdymercury I'm sorry, I totally don't get what you are saying. Where do you run that command, for me it looks very much different. And why would we want to add _SILENCE_CXX17_CODECVT_HEADER_DEPRECATION_WARNING? I argue that we can perfectly live (in Cling) with the implementation using LLVM's conversion functions. Other uses of <codecvt> are not subject of this PR and I don't want to spend my time on them here.

Nope, I was rather suggesting to remove the -D_SILENCE_CXX17_CODECVT_HEADER_DEPRECATION_WARNING from interpreter/cling/CMakeLists.txt:241, not to add it, but never mind, it might have side effects if it's being used in other places.

@hahnjo hahnjo merged commit b20c095 into root-project:master Feb 11, 2025
20 of 22 checks passed
@hahnjo hahnjo deleted the cling-deprecated branch February 11, 2025 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants