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 _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
