Bugzilla – Bug 2149
deprecated Attributes and TraceSources
Last modified: 2016-09-10 17:31:52 EDT
Created attachment 2079 [details] patch When we move attributes (and trace sources) to different classes, users of the old attribute path in the config system who haven't updated their example programs may silently fail to connect to anything. This code is proposed by Natale similar to how we use NS_DEPRECATED macro, in that we would leave a DeprecatedAttribute in the previous class, for a few release cycles, so that user programs using the old paths will notice the change.
Two questions: 1. Is this intended to block execution, as NS_DEPRECATED blocks (the first) compilation? or just warn for the future, but still succeed, using the (now deprecated) attribute? If the intent is to block execution, perhaps because there is no sensible way to proceed (e.g. the old attribute no longer exists), then should we be calling NS_ABORT_MSG_IF instead? 2. I'm not sure I understand the complexity here. Could we add a deprecated flag to AttributeInformation and TraceSourceInformation? Then add a default parameter "const bool deprecated = false" to the relevant AddAttribute and AddTraceSource signatures? The implementation would be much simpler overall, touching only a few signatures, not doubling the API. It would also be more efficient, since LookupAttributeByName/LookupTraceSourceByName would only traverse one list, the attributes or trace sources. Actually, it would be even better to make the deprecated flag a string, which should give a hint about how to update the user's code: struct AttributeInformation { ... bool deprecated; /**< If \c true this is attribute deprecated. */ std::string alternate; /**< If deprecated, the alternate message will give a hint to the new implementation. */ }; TypeId::AddAttribute ( ..., const std::string deprecated = ""); 406 void 407 IidManager::AddAttribute (uint16_t uid, 408 std::string name, 409 std::string help, 410 uint32_t flags, 411 Ptr<const AttributeValue> initialValue, 412 Ptr<const AttributeAccessor> accessor, 413 Ptr<const AttributeChecker> checker, const std::string alternate) 414 { 415 NS_LOG_FUNCTION (this << uid << name << help << flags << initialValue << accessor << checker); 416 struct IidInformation *information = LookupInformation (uid); 417 if (HasAttribute (uid, name)) 418 { 419 NS_FATAL_ERROR ("Attribute \"" << name << "\" already registered on tid=\"" << 420 information->name << "\""); 421 } 422 struct TypeId::AttributeInformation info; 423 info.name = name; 424 info.help = help; 425 info.flags = flags; if (alternate != "") { info.deprecated = true; info.alternate = alternate; } else info.deprecated = false; 426 info.initialValue = initialValue; 427 info.originalInitialValue = initialValue; 428 info.accessor = accessor; 429 info.checker = checker; 430 information->attributes.push_back (info); 431 } 609 TypeId::LookupAttributeByName (std::string name, struct TypeId::AttributeInformation *info) const 610 { 611 NS_LOG_FUNCTION (this << name << info); 612 TypeId tid; 613 TypeId nextTid = *this; 614 do { 615 tid = nextTid; 616 for (uint32_t i = 0; i < tid.GetAttributeN (); i++) 617 { 618 struct TypeId::AttributeInformation tmp = tid.GetAttribute(i); 619 if (tmp.name == name) 620 { NS_ABORT_MSG_IF (tmp.deprecated, "ATTENTION: Deprecated attribute " << name << ". " << tmp.alternate ); 621 *info = tmp; 622 return true; 623 } 624 } 625 nextTid = tid.GetParent (); 626 } while (nextTid != tid); 627 return false; 628 }
(In reply to Peter Barnes from comment #1) > 1. We should definitely not block compilation, but we absolutely should fail to execute the user's script. Warning the user and then handling the transition for him is nice, but my concern is that we will have trouble maintaining this over long term. > 2. +1, I like this better than original patch.
> > 2. > +1, I like this better than original patch. +2
(In reply to Vedran Miletić from comment #2) > (In reply to Peter Barnes from comment #1) > We should definitely not block compilation, but we absolutely should fail to > execute the user's script. Warning the user and then handling the transition > for him is nice, but my concern is that we will have trouble maintaining > this over long term. Well, to my mind "deprecated" means it will go away in about a year, so these won't be permanent maintenance headaches. I understand that in some cases refactoring may make it impossible to handle an old attribute. Perhaps we need to extend the simple "string deprecated" argument to AddAttribute (..., std::string alternate = "", bool fatal = false); with the obvious implementation changes that follow.
What do you think about that? Pratically, there is an enum value which indicates the level of the deprecation (default active) and an optional message. diff --git i/src/core/model/type-id.cc w/src/core/model/type-id.cc index 31d4a99..6cbe53d 100644 --- i/src/core/model/type-id.cc +++ w/src/core/model/type-id.cc @@ -101,7 +101,9 @@ public: uint32_t flags, Ptr<const AttributeValue> initialValue, Ptr<const AttributeAccessor> spec, - Ptr<const AttributeChecker> checker); + Ptr<const AttributeChecker> checker, + TypeId::DeprecationLevel level = ACTIVE, + const std::string &msg = ""); void SetAttributeInitialValue(uint16_t uid, uint32_t i, Ptr<const AttributeValue> initialValue); @@ -111,7 +113,9 @@ public: std::string name, std::string help, Ptr<const TraceSourceAccessor> accessor, - std::string callback); + std::string callback, + TypeId::DeprecationLevel level = TypeId::ACTIVE, + const std::string &msg = ""); uint32_t GetTraceSourceN (uint16_t uid) const; struct TypeId::TraceSourceInformation GetTraceSource(uint16_t uid, uint32_t i) const; bool MustHideFromDocumentation (uint16_t uid) const; @@ -417,7 +421,9 @@ IidManager::AddAttribute (uint16_t uid, uint32_t flags, Ptr<const AttributeValue> initialValue, Ptr<const AttributeAccessor> accessor, - Ptr<const AttributeChecker> checker) + Ptr<const AttributeChecker> checker, + TypeId::DeprecationLevel level, + const std::string &msg) { NS_LOG_FUNCTION (this << uid << name << help << flags << initialValue << accessor << checker); struct IidInformation *information = LookupInformation (uid); @@ -434,6 +440,8 @@ IidManager::AddAttribute (uint16_t uid, info.originalInitialValue = initialValue; info.accessor = accessor; info.checker = checker; + info.level = level; + info.msg = msg; information->attributes.push_back (info); } void @@ -498,7 +506,9 @@ IidManager::AddTraceSource (uint16_t uid, std::string name, std::string help, Ptr<const TraceSourceAccessor> accessor, - std::string callback) + std::string callback, + TypeId::DeprecationLevel level, + const std::string &msg) { NS_LOG_FUNCTION (this << uid << name << help << accessor); struct IidInformation *information = LookupInformation (uid); @@ -512,6 +522,8 @@ IidManager::AddTraceSource (uint16_t uid, source.help = help; source.accessor = accessor; source.callback = callback; + source.level = level; + source.msg = msg; information->traceSources.push_back (source); } uint32_t @@ -625,6 +637,15 @@ TypeId::LookupAttributeByName (std::string name, struct TypeId::AttributeInforma struct TypeId::AttributeInformation tmp = tid.GetAttribute(i); if (tmp.name == name) { + if (tmp.level == TypeId::DEPRECATED) + { + NS_LOG_UNCOND (msg); + /** should return true or false? */ + } + else if (tmp.level == TypeId::NOT_USED) + { + NS_FATAL_ERROR (msg); + } *info = tmp; return true; } @@ -727,13 +748,16 @@ TypeId::DoAddConstructor (Callback<ObjectBase *> cb) TypeId TypeId::AddAttribute (std::string name, - std::string help, + std::string help, const AttributeValue &initialValue, Ptr<const AttributeAccessor> accessor, - Ptr<const AttributeChecker> checker) + Ptr<const AttributeChecker> checker, DeprecationLevel level, + const std::string &msg) { NS_LOG_FUNCTION (this << name << help << &initialValue << accessor << checker); - Singleton<IidManager>::Get ()->AddAttribute (m_tid, name, help, ATTR_SGC, initialValue.Copy (), accessor, checker); + Singleton<IidManager>::Get ()->AddAttribute (m_tid, name, help, ATTR_SGC, + initialValue.Copy (), accessor, + checker, level, msg); return *this; } @@ -743,10 +767,13 @@ TypeId::AddAttribute (std::string name, uint32_t flags, const AttributeValue &initialValue, Ptr<const AttributeAccessor> accessor, - Ptr<const AttributeChecker> checker) + Ptr<const AttributeChecker> checker, DeprecationLevel level, + const std::string &msg) { NS_LOG_FUNCTION (this << name << help << flags << &initialValue << accessor << checker); - Singleton<IidManager>::Get ()->AddAttribute (m_tid, name, help, flags, initialValue.Copy (), accessor, checker); + Singleton<IidManager>::Get ()->AddAttribute (m_tid, name, help, flags, + initialValue.Copy (), accessor, + checker, level, msg); return *this; } @@ -822,10 +849,11 @@ TypeId TypeId::AddTraceSource (std::string name, std::string help, Ptr<const TraceSourceAccessor> accessor, - std::string callback) + std::string callback, DeprecationLevel level, const std::string &msg) { NS_LOG_FUNCTION (this << name << help << accessor); - Singleton<IidManager>::Get ()->AddTraceSource (m_tid, name, help, accessor, callback); + Singleton<IidManager>::Get ()->AddTraceSource (m_tid, name, help, accessor, + callback, level, msg); return *this; } @@ -851,6 +879,15 @@ TypeId::LookupTraceSourceByName (std::string name) const struct TypeId::TraceSourceInformation info = tid.GetTraceSource (i); if (info.name == name) { + if (info.level == TypeId::DEPRECATED) + { + NS_LOG_UNCOND (msg); + } + else if (info.level == TypeId::NOT_USED) + { + NS_FATAL_ERROR (msg); + } + return info.accessor; } } diff --git i/src/core/model/type-id.h w/src/core/model/type-id.h index 57de930..9ed237e 100644 --- i/src/core/model/type-id.h +++ w/src/core/model/type-id.h @@ -66,6 +66,15 @@ public: ATTR_CONSTRUCT = 1<<2, /**< The attribute can be written at construction-time */ ATTR_SGC = ATTR_GET | ATTR_SET | ATTR_CONSTRUCT, /**< The attribute can be read, and written at any time */ }; + /** + * \brief The level of deprecation of attribute or trace sources + */ + enum DeprecationLevel + { + ACTIVE, //!< Attribute or trace source is currently used + DEPRECATED, //!< Attribute or trace source is deprecated; user is warned + NOT_USED //!< Attribute or trace source is not used anymore; simulation fails + }; struct AttributeInformation { std::string name; std::string help; @@ -74,12 +83,16 @@ public: Ptr<const AttributeValue> initialValue; Ptr<const AttributeAccessor> accessor; Ptr<const AttributeChecker> checker; + TypeId::DeprecationLevel level; + const std::string &msg; }; struct TraceSourceInformation { std::string name; std::string help; std::string callback; Ptr<const TraceSourceAccessor> accessor; + TypeId::DeprecationLevel level; + const std::string &msg; }; /** @@ -293,7 +306,9 @@ public: std::string help, const AttributeValue &initialValue, Ptr<const AttributeAccessor> accessor, - Ptr<const AttributeChecker> checker); + Ptr<const AttributeChecker> checker, + DeprecationLevel level = ACTIVE, + const std::string &msg = ""); /** * \param i the attribute to manipulate @@ -320,7 +335,9 @@ public: uint32_t flags, const AttributeValue &initialValue, Ptr<const AttributeAccessor> accessor, - Ptr<const AttributeChecker> checker); + Ptr<const AttributeChecker> checker, + DeprecationLevel level = ACTIVE, + const std::string &msg = ""); /** * \param name the name of the new trace source @@ -350,7 +367,9 @@ public: TypeId AddTraceSource (std::string name, std::string help, Ptr<const TraceSourceAccessor> accessor, - std::string callback); + std::string callback, + DeprecationLevel level = ACTIVE, + const std::string &msg = ""); TypeId HideFromDocumentation (void);
Anyway there is a problem in the previous patch: how I can indicate a variable which isn't anymore a member of the class? Clearly, the same API cannot be mantained. Hints?
(In reply to natale.patriciello from comment #6) > Anyway there is a problem in the previous patch: how I can indicate a > variable which isn't anymore a member of the class? Clearly, the same API > cannot be mantained. Hints? I'd suggest creating a dummy variable for it.
(In reply to natale.patriciello from comment #6) > Anyway there is a problem in the previous patch: how I can indicate a > variable which isn't anymore a member of the class? Clearly, the same API > cannot be mantained. Hints? I thought that was the point of DeprecationLevel::NOT_USED: there is no longer a simple way for the code to do the right thing, so it's a fatal error. I suggest changing the names a bit: DeprecationLevel -> SupportLevel NOT_USED -> OBSOLETE TypeId::DeprecationLevel level -> supportLevel const std::string &msg -> supportMsg NS_LOG_UNCOND (msg); -> NS_LOG_UNCOND ("Attribute [or TraceSource] is deprecated: " << supportMsg); NS_FATAL_ERROR (msg); -> NS_FATAL_ERROR ("Attribute [or TraceSource] is obsolete, with no fallback: " << supportMsg); The point of the prefix on the error messages is to make sure the boilerplate information is conveyed. It also looks some doxygen is missing for the new arguments. For supportMsg, I would emphasize that it should indicate how to refactor the code to the current facilities.
Created attachment 2094 [details] Deprecation patch, with Peter's comments, and MakeNull*
(In reply to natale.patriciello from comment #6) > Anyway there is a problem in the previous patch: how I can indicate a > variable which isn't anymore a member of the class? Clearly, the same API > cannot be mantained. Hints? Thank you! I've included the comments from Peter, and also, I've added a MakeNullTraceSourceAccessor () and MakeNullAttributeAccessor () function. In this way, we are not forced to introduce dummy variables. What's your opinion? Nat
This looks good. I presume the MakeNull... functions are for cases where the supportLevel is OBSOLETE? So that there is no accessor? What about using EmptyAttributeValue instead? I don't understand the signature: template <typename T> Ptr<const TraceSourceAccessor> MakeNullTraceSourceAccessor (T a); Shouldn't this be as you have below: static inline Ptr<const TraceSourceAccessor> MakeNullTraceSourceAccessor (); To parallel EmptyAttributeValue, should we call this EmptyTraceSource, etc? Minor word-smithing: + * \param supportMsg If Attribute is DEPRECATED, this message should indicate how + * to refactor user code to the current facilities. The name of the + * deprecated Attribute is already printed.
(In reply to Peter Barnes from comment #11) > I presume the MakeNull... functions are for cases where the supportLevel is > OBSOLETE? So that there is no accessor? What about using > EmptyAttributeValue instead? My idea on the support level is the following, which applies perfectly on my use-case of moving cwnd and ssth in tcpsocketbase: -> deprecate is used when the attribute/trace is moved or it has no effect on the simulation/isn't traced anymore. However the simulation should continue, and the user warned. Eg, moving cwnd from NewReno to TcpSocketBase. -> obsolete is when the attribute/trace has been deprecated for a while (2 releases?) and the simulation now should be stopped, because if the user continues to utilize that attribute/trace, he/she probably should take actions. In both cases, in the class the member variable does not exist anymore (eg m_cWnd in TcpNewReno). Here, it comes my patch; while the signature could remain the same, MakeNull* resolve the compilation errors complaining the absence of the attribute/trace accessor. As rule, we can state that EmptyAttributeValue should be used instead of the original AttributeValue subclass (and, in the same manner, an EmptyTraceSource could be created and used). Anyway, MakeNull* accessor should be used. What's your opinion on that? How would you handle the move of cwnd and ssth in TcpSocketBase? > I don't understand the signature: > > template <typename T> > Ptr<const TraceSourceAccessor> MakeNullTraceSourceAccessor (T a); > > Shouldn't this be as you have below: > > static inline > Ptr<const TraceSourceAccessor> MakeNullTraceSourceAccessor (); Yes, my fault (I've uploaded an old version of the patch) > To parallel EmptyAttributeValue, should we call this EmptyTraceSource, etc? Of course > Minor word-smithing: > > + * \param supportMsg If Attribute is DEPRECATED, this message should > indicate how > + * to refactor user code to the current facilities. The name of the > + * deprecated Attribute is already printed. Thank you! I'm preparing a patch with these fixes, I'll upload it as soon as possible. Nat
Created attachment 2098 [details] Deprecation patch re-worked I added the EmptyAttribute Accessor and checked. They are used like this: .AddAttribute ("ReTxThreshold", "Threshold for fast retransmit", - UintegerValue (3), - MakeUintegerAccessor (&TcpNewReno::m_retxThresh), - MakeUintegerChecker<uint32_t> ()) + EmptyAttributeValue (), + MakeEmptyAttributeAccessor (), + MakeEmptyAttributeChecker (), + TypeId::DEPRECATED, + "Moved inside TcpSocketBase") For TraceSource, we have: + .AddTraceSource ("CongestionWindow", + "The TCP connection's congestion window", + MakeEmptyTraceSourceAccessor (), + "ns3::TracedValue::VoidCallback", + TypeId::DEPRECATED, + "Moved inside TcpSocketBase")
I don't think we're clear yet on expected behavior for the DEPRECATED case. I don't understand > deprecate is used when the attribute/trace is moved or it has no effect on the > simulation/isn't traced anymore" Let's consider these in turn: Attribute moved: DEPRECATED, and rewrite AddAttribute to point to the new member data. Notice that the code still *implements* the expected effect, so users get the model behavior they expect, even using the old attribute. This case doesn't need MakeEmpty... Attribute has no effect: if it *used* to have an effect, but can't be made to have the same effect now, then this should be OBSOLETE and raise an error. I presume we don't define attributes which have no effects! (Though I think there a few "not yet implemented") Here we use MakeEmpty... to compile, but we fatal error if it is ever used. Trace moved: DEPRECATED, and fix up, same as Attribute moved. Trace no longer available: OBSOLETE and error. This too must raise an error, since the expected events and data can't be provided to code expecting it. The point is: if the behavior can be preserved, then DEPRECATED is ok. Any change in behavior must OBSOLETE the attribute/trace.
(In reply to Peter Barnes from comment #14) > Attribute has no effect: if it *used* to have an effect, but can't be made > to have the same effect now, then this should be OBSOLETE and raise an > error. I presume we don't define attributes which have no effects! (Though > I think there a few "not yet implemented") Here we use MakeEmpty... to > compile, but we fatal error if it is ever used. Agree. > Trace no longer available: OBSOLETE and error. This too must raise an > error, since the expected events and data can't be provided to code > expecting it. Agree also for this. I'll add these on the doxygen documentation. > Attribute moved: DEPRECATED, and rewrite AddAttribute to point to the new > member data. Notice that the code still *implements* the expected effect, > so users get the model behavior they expect, even using the old attribute. > This case doesn't need MakeEmpty... Uhm, I'm not sure about this. Let's define attribute "Attr" which was in class A, and now is in class B. Since the attribute is moved, the member variable could not be accessible by the class A; so, class A AddAttribute requires the use of MakeEmpty(), because physically the member variable is in class B.
> > > Attribute moved: DEPRECATED, and rewrite AddAttribute to point to the new > > member data. Notice that the code still *implements* the expected effect, > > so users get the model behavior they expect, even using the old attribute. > > This case doesn't need MakeEmpty... > > Uhm, I'm not sure about this. Let's define attribute "Attr" which was in > class A, and now is in class B. Since the attribute is moved, the member > variable could not be accessible by the class A; so, class A AddAttribute > requires the use of MakeEmpty(), because physically the member variable is > in class B. I think I agree with both of you; is it a terminology issue? How about this further breakdown. Attribute moved: case 1: If the attribute (renamed within same class, or private member moved to another class) can be accessed via the old name i) without change of semantics or behavior, and ii) without adding cruft to preserve linkage (e.g. making friend classes), then DEPRECATE. case 2: Otherwise, OBSOLETE/error. Similarly for trace sources (only deprecate if can be made to work without too much hassle at the old name/location)... FWIW I agree with the rest of Peter's taxonomy: https://www.nsnam.org/bugzilla/show_bug.cgi?id=2149#c14
(In reply to Tom Henderson from comment #16) > I think I agree with both of you; is it a terminology issue? How about this > further breakdown. > > Attribute moved: > case 1: If the attribute (renamed within same class, or private member > moved to another class) can be accessed via the old name i) without change > of semantics or behavior, and ii) without adding cruft to preserve linkage > (e.g. making friend classes), then DEPRECATE. > > case 2: Otherwise, OBSOLETE/error. > > Similarly for trace sources (only deprecate if can be made to work without > too much hassle at the old name/location)... Completely agree.
Created attachment 2562 [details] Revised patch updated to 2016-09-02 Updated to r12295 [e1451373c0b6]
Created attachment 2563 [details] Updated to r12295 [e1451373c0b6]
Created attachment 2564 [details] Revised patch over r12299 [ea2d7890a6da] Revised patch over r12299 [ea2d7890a6da]
Sorry for so many patches. I had trouble/was sloppy in disentangling TypeId logging I used for debugging and this deprecation. I think I've got it right now.
pushed in 12314:7224ff0eb8d9