Skip to content

Unchecked Expected introduced in 3706588 #3505

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.

Sign up for GitHub

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

Jump to bottom
Closed
chadrosier opened this issue Sep 12, 2019 · 3 comments
Closed

Unchecked Expected introduced in 3706588 #3505

chadrosier opened this issue Sep 12, 2019 · 3 comments
Assignees
@beicy

Comments

@chadrosier
Copy link
Contributor

The below commit introduced a number of unchecked llvm::Expected errors:
Partitioner refactoring 3 (#3384)

glow/tests/unittests/PartitionerTest.cpp

Line 252 in d12d6d0

auto dagList = myPartitioner.partition(cctx);

Needs something like EXPECT_TRUE((bool)dagList);

glow/tests/unittests/PartitionerTest.cpp

Line 346 in d12d6d0

EXPECT_FALSE((bool)dagList);

IIUC, this is a negative test with an Error. The bool operator for Expected sets Unchecked = true if there's an actual Error. Therefore, this expect remains unchecked.

glow/tests/unittests/PartitionerTest.cpp

Line 893 in d12d6d0

EXPECT_FALSE((bool)dagList);

Same as previous.

glow/tests/unittests/PartitionerTest.cpp

Line 922 in d12d6d0

EXPECT_FALSE((bool)dagList);

Same as previous.

glow/tests/unittests/PartitionerTest.cpp

Line 954 in d12d6d0

EXPECT_FALSE((bool)dagList);

Same as previous.

This requires a build of LLVM with asserts enabled.

Would you mind taking a look @beicy?
@beicy
Copy link
Contributor

beicy commented Sep 12, 2019

Sure! Thanks! @chadrosier

@beicy beicy self-assigned this Sep 12, 2019
@chadrosier
Copy link
Contributor Author

See also: #2737

@beicy beicy mentioned this issue Sep 13, 2019
Closed
@jackm321 jackm321 mentioned this issue Sep 16, 2019
Closed
facebook-github-bot pushed a commit that referenced this issue Sep 16, 2019
@facebook-github-bot
Fix Unchecked errors in Partitioner #3505 (#3511)
16dd2fd 
Summary:
Fix unchecked errors in Partitioner, rewrite the way to report an error.
#3505
Documentation:

[Optional Fixes]
Pull Request resolved: #3511

Test Plan:
ninja test.
Please see a detailed explanation of how to fill out the fields in the relevant sections in PULL_REQUEST.md.

Differential Revision: D17403326

Pulled By: beicy

fbshipit-source-id: fbab4dbf1d235e148e40b344665459533f60fdde
jackm321 added a commit to jackm321/glow that referenced this issue Sep 17, 2019
@jackm321 @facebook-github-bot
Create glow::Error and glow::Expected (pytorch#3515)
5d356c8 
Summary:
_Note for review: Most real changes are in `Error.h`, `Error.cpp`, and `ErrorTest.cpp` and the rest is mostly codemod to switch from `llvm::Error/Expected` to `glow::Error/Expected`._
Removes dependency on `llvm::Error` and ensures Errors are don't go unchecked by verifying this in debug build.

`glow::Error/Expected` are mostly drop-in replacements for `llvm::Error/Expected` but exclude some features not used in Glow such as multiple error value types, reference types contained in Expected, etc. They also have a couple of different features like `Error::empty()` which creates "pre-checked" `Error`s.

Documentation:
doxygen
fixes: pytorch#2737, pytorch#3505
Pull Request resolved: pytorch#3515

Test Plan:
added `ErrorTest` unit tests
ninja check

Differential Revision: D17399297

fbshipit-source-id: 4b0a524e2c1ba2cf4adc766a0e8196ee94e062f0
facebook-github-bot pushed a commit that referenced this issue Sep 17, 2019
@jackm321 @facebook-github-bot
Create glow::Error and glow::Expected (#3515)
cca5793 
Summary:
_Note for review: Most real changes are in `Error.h`, `Error.cpp`, and `ErrorTest.cpp` and the rest is mostly codemod to switch from `llvm::Error/Expected` to `glow::Error/Expected`._
Removes dependency on `llvm::Error` and ensures Errors are don't go unchecked by verifying this in debug build.

`glow::Error/Expected` are mostly drop-in replacements for `llvm::Error/Expected` but exclude some features not used in Glow such as multiple error value types, reference types contained in Expected, etc. They also have a couple of different features like `Error::empty()` which creates "pre-checked" `Error`s.

Documentation:
doxygen
fixes: #2737, #3505
Pull Request resolved: #3515

Test Plan:
added `ErrorTest` unit tests
ninja check

Reviewed By: opti-mix

Differential Revision: D17399297

Pulled By: jackm321

fbshipit-source-id: 5f57aaae8c18684d182b35f5801262778b6c60f4
@chadrosier
Copy link
Contributor Author

Fixed in 16dd2fd.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@beicy beicy

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@chadrosier @beicy