Task #1983
closed
Ensure Element::Error inherits from tlv::Error
Added by Junxiao Shi over 9 years ago.
Updated almost 9 years ago.
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).
- Description updated (diff)
- Status changed from New to In Progress
- Assignee set to Junxiao Shi
- % Done changed from 0 to 50
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?
- Description updated (diff)
- % Done changed from 50 to 80
- Status changed from In Progress to Code review
- % Done changed from 80 to 100
- Status changed from Code review to Closed
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.
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.
Also available in: Atom
PDF
Updated by 
Updated by 
Updated by