Skip to content
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

[1/N] Add -Wdeprecated and related fixes #108626

Closed
wants to merge 3 commits into from

Conversation

cyyever
Copy link
Collaborator

@cyyever cyyever commented Sep 6, 2023

This PR adds -Wdeprecated to CMake warnings and fixes related issues.
cc @EikanWang @jgong5 @ezyang @colesbury @Skylion007

@pytorch-bot
Copy link

pytorch-bot bot commented Sep 6, 2023

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/108626

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit 63a111e with merge base 2c1554a (image):
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@pytorch-bot pytorch-bot bot added the topic: not user facing topic category label Sep 6, 2023
@cyyever cyyever marked this pull request as draft September 6, 2023 06:44
@cyyever cyyever changed the title Add Wdeprecated to warnings Add Wdeprecated and Wpendantic Sep 8, 2023
@cyyever cyyever force-pushed the wdeprecated branch 4 times, most recently from 53d6177 to fab245d Compare September 8, 2023 12:01
@cyyever cyyever changed the title Add Wdeprecated and Wpendantic Add Wdeprecated Sep 8, 2023
@cyyever cyyever force-pushed the wdeprecated branch 19 times, most recently from f8de1e1 to 2c34e4b Compare September 11, 2023 13:39
@pytorchmergebot
Copy link
Collaborator

Successfully rebased wdeprecated onto refs/remotes/origin/main, please pull locally before adding more changes (for example, via git checkout wdeprecated && git pull --rebase)

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

@cyyever
Copy link
Collaborator Author

cyyever commented Sep 19, 2023

@pytorchbot label ciflow/binaries

@pytorch-bot pytorch-bot bot added the ciflow/binaries Trigger all binary build and upload jobs on the PR label Sep 19, 2023
@pytorchmergebot
Copy link
Collaborator

The merge job was canceled. If you believe this is a mistake, then you can re trigger it through pytorch-bot.

@cyyever
Copy link
Collaborator Author

cyyever commented Sep 19, 2023

@pytorchmergebot merge

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

@clee2000
Copy link
Contributor

clee2000 commented Sep 20, 2023

@pytorchbot revert -m "I'm getting errors internally that look like the below on x86_64-apple-ios-simulator with clang 16 D49440218" -c ghfirst

xplat/caffe2/c10/util/Optional.h:740:12: error: implicit conversion turns string literal into bool: 'const char[14]' to 'bool' [-Werror,-Wstring-conversion]
    return TR2_OPTIONAL_ASSERTED_EXPRESSION(initialized(), dataptr());
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                                                 ^

@pytorchmergebot
Copy link
Collaborator

@pytorchbot successfully started a revert job. Check the current status here.
Questions? Feedback? Please reach out to the PyTorch DevX Team

@pytorchmergebot
Copy link
Collaborator

@cyyever your PR has been successfully reverted.

pytorchmergebot added a commit that referenced this pull request Sep 20, 2023
This reverts commit a53a677.

Reverted #108626 on behalf of https://github.com/clee2000 due to I'm getting errors internally that look like the below on x86_64-apple-ios-simulator with clang 16 ([comment](#108626 (comment)))
@cyyever
Copy link
Collaborator Author

cyyever commented Sep 20, 2023

@clee2000 Should we put x86_64-apple-ios-simulator tests in CI?

implicit conversion turns string literal into bool: 'const char[14]' to 'bool' 

May I obtain the string literal because there is no string involved in c10/util/Optional.h

@clee2000
Copy link
Contributor

clee2000 commented Sep 21, 2023

I'm not sure what the string literal is tbh

@ezyang says

TR2_OPTIONAL_HOST_CONSTEXPR T const& operator*() const& {
    return TR2_OPTIONAL_ASSERTED_EXPRESSION(initialized(), contained_val());
  }

is the critical line.

This is what the rest of the error looks like, I've changed some things since I'm not sure what I'm actually allowed to post

In file included from ***
In file included from caffe2/core/operator.h:21:
In file included from caffe2/core/blob.h:14:
In file included from caffe2/core/tensor.h:5:
In file included from caffe2/core/storage.h:23:
In file included from c10/core/Storage.h:3:
In file included from c10/core/StorageImpl.h:4:
In file included from c10/core/SymInt.h:3:
In file included from c10/core/SymBool.h:3:
In file included from c10/core/SymNodeImpl.h:6:
c10/util/Optional.h:749:12: error: implicit conversion turns string literal into bool: 'const char[14]' to 'bool' [-Werror,-Wstring-conversion]
    return TR2_OPTIONAL_ASSERTED_EXPRESSION(initialized(), contained_val());
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
c10/util/Optional.h:87:37: note: expanded from macro 'TR2_OPTIONAL_ASSERTED_EXPRESSION'
  ((CHECK) ? (EXPR) : ([] { assert(!#CHECK); }(), (EXPR)))
                                   ~^~~~~~
<scratch space>:85:1: note: expanded from here
"initialized()"
^~~~~~~~~~~~~~~
***/third-party/toolchains/xcode/14.1.0/MacOSX.sdk/usr/include/assert.h:99:25: note: expanded from macro 'assert'
    (__builtin_expect(!(e), 0) ? __assert_rtn(__func__, __ASSERT_FILE_NAME, __LINE__, #e) : (void)0)
                        ^
c10/util/Optional.h:782:20: note: in instantiation of member function 'c10::optional<bool>::operator*' requested here
    return *this ? **this : detail_::convert<T>(constexpr_forward<V>(v));
                   ^
c10/core/TensorOptions.h:42:24: note: in instantiation of function template specialization 'c10::optional<bool>::value_or<bool>' requested here
  return pinned_memory.value_or(false);
                       ^
6 errors generated.
    When running <c++ preprocess_and_compile>.
    When building rule ***
Not all rules succeeded.

Side note do you have slack?

I thought the ios simulator tests would have been enough but I guess not? @malfet

cc @ezyang

@cyyever
Copy link
Collaborator Author

cyyever commented Sep 21, 2023

@clee2000 Thank you so much,very helpful track back information. I will try to fix it tomorrow

@malfet
Copy link
Contributor

malfet commented Sep 21, 2023

@clee2000 our iOS build are running using different version of xcode, this is why it was not caught

pytorchmergebot pushed a commit that referenced this pull request Sep 28, 2023
This is reland of PRs ##108626 and #109564. We fixed the IOS build failure by changing
```
((CHECK) ? (EXPR) : ([] { assert(!#CHECK); }(), (EXPR)))
```
to
```
((CHECK) ? (EXPR) : ([] { assert(false); }(), (EXPR)))
```
in TR2_OPTIONAL_ASSERTED_EXPRESSION, since the former syntax was invalid on Apple Clang. Anyway, we could apply the simple fix hoping that c10::optional would be replaced by std::optional soon.
We also enabled -Wdeprecated on c10.

Pull Request resolved: #110019
Approved by: https://github.com/clee2000
@cyyever cyyever deleted the wdeprecated branch September 30, 2023 01:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ciflow/binaries Trigger all binary build and upload jobs on the PR ciflow/trunk Trigger trunk jobs on your pull request Merged NNC open source Reverted topic: not user facing topic category triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants