Task #1983
closed
Ensure Element::Error inherits from tlv::Error
100%
Description
Most decoding procedure assumes that Element::Error inherits from tlv::Error.
Such decoding procedure looks like:
try {
interest.wireDecode(block);
}
catch (tlv::Error&) {
// handle error
}
This Task is to add BOOST_STATIC_ASSERT to each TLV elememt abstraction type, so that Element::Error is guaranteed to inherit from tlv::Error.
One such assert looks like:
static_assert(std::is_base_of<tlv::Error, Data::Error>::value,
"Data::Error must inherit from tlv::Error");
This line should appear in .cpp for the implementation (not in .hpp or unit testing).
Updated by Junxiao Shi over 9 years ago
- Status changed from New to In Progress
- Assignee set to Junxiao Shi
Updated by Junxiao Shi over 9 years ago
- % Done changed from 0 to 50
Elements in src/ http://gerrit.named-data.net/1384
Updated by Junxiao Shi over 9 years ago
This Task is useful, because it does catch some Element::Error not deriving from tlv::Error.
I notice that Name::Error derives from name::Component::Error, and name::Component::Error derives from Block::Error.
Although inheritance is correct, I wonder what's rationale of this design?
Updated by Junxiao Shi over 9 years ago
- Blocked by Task #2132: code-style: assert added
Updated by Junxiao Shi over 9 years ago
- % Done changed from 50 to 80
management/ http://gerrit.named-data.net/1456
Updated by Junxiao Shi over 9 years ago
- Status changed from In Progress to Code review
- % Done changed from 80 to 100
security/ http://gerrit.named-data.net/1496
Updated by Junxiao Shi over 9 years ago
- Status changed from Code review to Closed
Updated by Davide Pesavento almost 9 years ago
These static_asserts are completely useless in my opinion. From http://gerrit.named-data.net/#/c/2199/4/src/security/transform/sink-bool.cpp
Davide Pesavento Jul 5 23:41
all these static_asserts look extremely pointless to me
Junxiao Shi 00:16
They are necessary to prevent mistakes. see #1983 note-4 and #1528.
Davide Pesavento 01:20
These asserts don't prevent anything. The "mistakes" you're referring to should be caught through code review. Whenever someone introduces a new Error subclass, during code review you check that the inheritance is correct. But this assert here does NOT help in any way because it doesn't know anything about the new subclass.
What you can and should test (via a test case) is that an exception of the correct type is generated when an error condition arises. And there, if you like, you can also verify that it's a subclass of Filter::Error using an appropriate catch clause.
Updated by Alex Afanasyev over 8 years ago
I kind of agree with Davide that those checks are mostly for decoration, rather than any particula use. Though I don't see a compelling reason to remove those or move them to test cases.
I actually have more issues with other asserts (move constructable and others). I was not able produce a compilation error even when I was explicitly disabling move operations (with my clang). I guess those are also for decoration only.