Add DRAKE_DEPRECATED macro

[sherm1]

Add a portable macro for marking classes, functions, variables, etc. as deprecated.

---

This change is

[liangfok]

I'm going to assume for now that "status: do not review" implies "do not test" as well.

[sherm1]

Ready for review now. Feature: +@liangfok, platform: +@jwnimmer-tri please.

---

Review status: 0 of 5 files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[liangfok]

Awesome. Testing now.

---

Review status: 0 of 5 files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[liangfok]

I tested this and it works. However, can we add a SWIG scenario to the unit test? I was having issues with SWIG in #2597.

FTR, using ninja, I had to scroll up really far to see the following (good) warnings:

[31/260] Building CXX object common/test/CMakeFiles/drake_deprecated_test.dir/drake_deprecated_test.cc.o
/Users/liang/dev/drake/drake/common/test/drake_deprecated_test.cc:26:3: warning: 'MyClass' is deprecated:
DRAKE DEPRECATED: use MyNewClass instead [-Wdeprecated-declarations]
  MyClass this_is_obsolete;
  ^
/Users/liang/dev/drake/drake/common/test/drake_deprecated_test.cc:14:50: note: 'MyClass' has been explicitly marked deprecated here
class DRAKE_DEPRECATED("use MyNewClass instead") MyClass {
                                                 ^
/Users/liang/dev/drake/drake/common/test/drake_deprecated_test.cc:33:18: warning: 'f' is deprecated:
DRAKE DEPRECATED: don't use this function; use g() instead [-Wdeprecated-declarations]
  int obsolete = f(1);
                 ^
/Users/liang/dev/drake/drake/common/test/drake_deprecated_test.cc:21:5: note: 'f' has been explicitly marked deprecated here
int f(int arg) { return arg; }
    ^
2 warnings generated.

I suspect using make will require scrolling up even more since it is by default more verbose. Since the warnings are so hard to find even manually, this may motivate learning how to modify the unit test to check for compile-time warnings.

---

Reviewed 2 of 4 files at r1, 2 of 3 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 7 unresolved discussions.

---

drake/common/drake_deprecated.h, line 9 [r3] (raw file):

#ifdef DRAKE_DOXYGEN_CXX
/** Use `DRAKE_DEPRECATED("message")` to discourage use of particular
classes, typedefs, variables, non-static data members, functions, arguments,

Wow. I did not realize the deprecated macro could be used for so many different things. The unit tests only include classes and functions. Should typedefs, variables, non-static data members, arguments, enumerations, and template specialization also be covered by the unit tests?

---

drake/common/drake_deprecated.h, line 21 [r3] (raw file):

  // Attribute comes *before* declaration of a deprecated function or variable;
  // no semicolon is allowed.
  DRAKE_DEPRECATED("f() is slow; use g() instead")

Missing period at end of message.

---

drake/common/drake_deprecated.h, line 25 [r3] (raw file):

  // Attribute comes *after* struct, class, enum keyword.
  class DRAKE_DEPRECATED("use MyNewClass instead")

Missing period at end of message.

---

drake/common/drake_deprecated.h, line 41 [r3] (raw file):

#ifndef SWIG
  /* Make sure warnings are enabled in VC++; it is a common Windows hack for
  programmers to turn them off due to much inconvenient deprecation of useful

BTW, there seems to be a missing "too" in front of "much".

---

drake/common/test/drake_deprecated_test.cc, line 10 [r3] (raw file):

/* This test only verifies that the Drake build can still succeed if a 
deprecated class or function is in use. I do not know how to test whether
the desired warnings are actually issued except by inspection. */

BTW, I believe one way would be to have the unit test actually compile a small test program that calls a deprecated function and analyze the output of the compile process.

http://stackoverflow.com/questions/3803465/how-to-capture-stdout-stderr-with-googletest

@david-german-tri: Is this something we should try to do here for educational purposes? Is there a better way to use Google Test to evaluate compile-time output?

---

drake/common/test/drake_deprecated_test.cc, line 20 [r3] (raw file):

};

DRAKE_DEPRECATED("don't use this function; use g() instead")

Missing period at end of error message.

---

drake/common/test/drake_deprecated_test.cc, line 21 [r3] (raw file):

DRAKE_DEPRECATED("don't use this function; use g() instead")
int f(int arg) { return arg; }

BTW, I recall there being some recommendation against using single character variables because they don't convey much in the way of semantics. Consider non_deprecated_method() and deprecated_method() or new_method() and old_method().

---

Comments from Reviewable

[jwnimmer-tri]

Reviewed 2 of 4 files at r1, 2 of 3 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 8 unresolved discussions.

---

drake/common/drake_deprecated.h, line 51 [r3] (raw file):

    #define DRAKE_DEPRECATED(MSG) [[deprecated("\nDRAKE DEPRECATED: " MSG)]]
  #elif _MSC_VER
    /* VC++ just says warning C4996 so add "DEPRECATED" to the message. */

BTW all of the variants prefix "DRAKE DEPRECATED " in the now -- is this comment still what you want to say?

---

drake/common/test/drake_deprecated_test.cc, line 3 [r3] (raw file):

#include "drake/common/drake_deprecated.h"

#include <string>

Unused?

---

drake/common/test/drake_deprecated_test.cc, line 4 [r3] (raw file):

#include <string>
#include <vector>

Unused?

---

drake/common/test/drake_deprecated_test.cc, line 10 [r3] (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

BTW, I believe one way would be to have the unit test actually compile a small test program that calls a deprecated function and analyze the output of the compile process.

http://stackoverflow.com/questions/3803465/how-to-capture-stdout-stderr-with-googletest

@david-german-tri: Is this something we should try to do here for educational purposes? Is there a better way to use Google Test to evaluate compile-time output?

The `drake_assert_test_compile` is an example of how to unit test compiler failure via `CMakeLists.txt`. If it seemed worth it, a similar tactic could be used for `DRAKE_DEPRECATED` -- when `-Werror=deprecated-declarations` is enabled, a program fails to compile; with that warning turned off, it passes. Possible even `drake_deprecated_test.cc` could be the "doesn't compile anymore" target file.

---

drake/common/test/drake_deprecated_test.cc, line 32 [r3] (raw file):

}

GTEST_TEST(DrakeAssertDeathTest, FunctionTest) {

DrakeAssertDeathTest seems incorrect here.

---

Comments from Reviewable

[sherm1]

Review status: 3 of 5 files reviewed at latest revision, 8 unresolved discussions.

---

drake/common/drake_deprecated.h, line 9 [r3] (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

Wow. I did not realize the deprecated macro could be used for so many different things. The unit tests only include classes and functions. Should typedefs, variables, non-static data members, arguments, enumerations, and template specialization also be covered by the unit tests?

That would be a test of the compiler rather than the macro. I'm assuming the compilers implement the deprecated attribute correctly.

---

drake/common/drake_deprecated.h, line 21 [r3] (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

Missing period at end of message.

Done.

---

drake/common/drake_deprecated.h, line 25 [r3] (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

Missing period at end of message.

Done.

---

drake/common/drake_deprecated.h, line 41 [r3] (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

BTW, there seems to be a missing "too" in front of "much".

No, that's what I intended to say. No amount of "inconvenient deprecation" is good!

---

drake/common/drake_deprecated.h, line 51 [r3] (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW all of the variants prefix "DRAKE DEPRECATED " in the now -- is this comment still what you want to say?

Done.

---

drake/common/test/drake_deprecated_test.cc, line 3 [r3] (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Unused?

Done.

---

drake/common/test/drake_deprecated_test.cc, line 4 [r3] (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Unused?

Done.

---

drake/common/test/drake_deprecated_test.cc, line 10 [r3] (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

The drake_assert_test_compile is an example of how to unit test compiler failure via CMakeLists.txt. If it seemed worth it, a similar tactic could be used for DRAKE_DEPRECATED -- when -Werror=deprecated-declarations is enabled, a program fails to compile; with that warning turned off, it passes. Possible even drake_deprecated_test.cc could be the "doesn't compile anymore" target file.

That would be awesome but I'm going to declare this sufficiently tested and move on. I updated the comment in case someone wants to push it further, but stopped short of making it a TODO. I would welcome a PR with a more comprehensive test!

---

drake/common/test/drake_deprecated_test.cc, line 20 [r3] (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

Missing period at end of error message.

Done.

---

drake/common/test/drake_deprecated_test.cc, line 21 [r3] (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

BTW, I recall there being some recommendation against using single character variables because they don't convey much in the way of semantics. Consider non_deprecated_method() and deprecated_method() or new_method() and old_method().

Done.

---

drake/common/test/drake_deprecated_test.cc, line 32 [r3] (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

DrakeAssertDeathTest seems incorrect here.

Done. (I wonder where that came from :)

---

Comments from Reviewable

[sherm1]

All reviewer comments addressed. PTAL.

---

Review status: 3 of 5 files reviewed at latest revision, 11 unresolved discussions.

---

Comments from Reviewable

[liangfok]

Especially since I have a workaround for the SWIG issue in #2597 .

---

Reviewed 1 of 2 files at r4.
Review status: 4 of 5 files reviewed at latest revision, 7 unresolved discussions.

---

Comments from Reviewable

[liangfok]

Reviewed 1 of 2 files at r4.
Review status: all files reviewed at latest revision, 7 unresolved discussions.

---

Comments from Reviewable

[jwnimmer-tri]

---

Reviewed 2 of 2 files at r4.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable