[PATCH] cpp11-migrate: Add '-supports' command line switch.

Edwin Vane edwin.vane at intel.com
Wed Jul 24 09:15:08 PDT 2013


  Do you see any value in tests for the edge cases for -supports? Bad compiler names, bad version numbers, version numbers with patch components? Also, do any of the tests above test the case when no transforms get selected? There should probably be a special message to the user in that case.


================
Comment at: cpp11-migrate/Core/Transform.h:76
@@ -74,1 +75,3 @@
+  /// \brief Whether all transforms should be enabled by default or not.
+  bool EnableAllTransformByDefault;
 };
----------------
Transform -> Transforms.

================
Comment at: cpp11-migrate/Core/Transform.h:317
@@ +316,3 @@
+protected:
+  /// \brief Since when this transform is available.
+  ///
----------------
I'd say "Since when the C++11 feature introduced by this transform has been available."

================
Comment at: cpp11-migrate/Core/Transform.cpp:143
@@ +142,3 @@
+  if (!MinorStr.empty() && MinorStr.getAsInteger(10, V.Minor))
+    return Version();
+  if (MajorStr.getAsInteger(10, V.Major))
----------------
Do we care to gracefully handle the patch version #? (i.e. the Z in X.Y.Z) Right now this code looks like it will just fail out since MinorStr will be of the form Y.Z and won't be an integer.

================
Comment at: cpp11-migrate/tool/Cpp11Migrate.cpp:125
@@ +124,3 @@
+        "Currently supports:\n"
+        "\tc++11, clang-VERSION, gcc-VERSION, icc-VERSION and msvc-VERSION\n"
+        "where VERSION is MAJOR[.MINOR], example:\n"
----------------
Any reason to use \t instead of spaces?

I see the c++11 option is explained in the docs but it might benefit to have a small description. Something like:

Currently supports:
   c++11 (turn on all transforms), clang-VERSION, ...

================
Comment at: docs/MigratorUsage.rst:139
@@ +138,3 @@
+
+  (1): if *-override-macros* is provided it's assumed that the macros is C++11
+  aware and the transform is enabled without regard to the supported compilers.
----------------
is->are.

================
Comment at: docs/MigratorUsage.rst:119
@@ +118,3 @@
+  Select the platforms to support. The transforms will be selected automatically
+  to work on all selected platform.
+
----------------
platform->platforms.

================
Comment at: docs/MigratorUsage.rst:118
@@ +117,3 @@
+
+  Select the platforms to support. The transforms will be selected automatically
+  to work on all selected platform.
----------------
The transforms -> Transforms.

================
Comment at: docs/MigratorUsage.rst:126
@@ +125,3 @@
+
+  And four compilers are supported. The transforms are enabled according to this
+  table:
----------------
Remove 'And'.

================
Comment at: docs/MigratorUsage.rst:142
@@ +141,3 @@
+
+  The version has to be provided like this `-supports [COMPILER]-[VERSION]`.
+
----------------
I'd reword to:

The structure of the argument to the -supports option is <compiler>-<major ver>[.<minor ver>] where <compiler> is one of the compilers from the above table.

================
Comment at: docs/MigratorUsage.rst:146
@@ +145,3 @@
+
+  1. If we want to support `Clang >= 3`, `GCC >= 4.6` and `MSVC >= 11`:
+
----------------
Reword:

1. To suport ...

================
Comment at: docs/MigratorUsage.rst:153
@@ +152,3 @@
+
+  2. If we want to support `icc >= 12` and a C++11-aware macro exists for the
+     `override` identifier:
----------------
2. To support icc >= 12 while using a C++11-aware macro for the override virtual specifier:

================
Comment at: cpp11-migrate/tool/Cpp11Migrate.cpp:128
@@ +127,3 @@
+        "\tcpp11-migrate -toward clang-3.1 -toward gcc-4.7 -toward icc-12.1 "
+        "-toward msvc-11 <args...>"));
+
----------------
Still using -toward here.

================
Comment at: cpp11-migrate/tool/Cpp11Migrate.cpp:126
@@ +125,3 @@
+        "\tc++11, clang-VERSION, gcc-VERSION, icc-VERSION and msvc-VERSION\n"
+        "where VERSION is MAJOR[.MINOR], example:\n"
+        "\tcpp11-migrate -toward clang-3.1 -toward gcc-4.7 -toward icc-12.1 "
----------------
I'd remove the example. This is something better left for the docs.


http://llvm-reviews.chandlerc.com/D1202

BRANCH
  supports-cmd-2

ARCANIST PROJECT
  clang-tools-extra



More information about the cfe-commits mailing list