[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