alexfh requested changes to this revision. This revision now requires changes to proceed.
================ Comment at: clang-tidy/modernize/UseNoexceptCheck.cpp:37 @@ +36,3 @@ +bool UseNoexceptCheck::checkHelper(const MatchFinder::MatchResult &Result, + const SourceRange &Range, + CharSourceRange &FileMoveRange, ---------------- > Happy to rename it, but not sure making it more convenient serves much of a > purpose. Well, the initial problem was that the `check` method was too long and complicated. I suggested to make it easier to read by pulling a part of it in a separate method. I pointed to a part that seemed like something worth extracting to a method, and you mechanically created a method with unclear responsibilities, an awkward interface (two output parameters, a bool return value and a class data member) and a name that doesn't help understanding what the method does. That doesn't make the code any better. Let's try once again. We need to pull out an abstraction (at least one, but maybe you see more) that will make sense on its own and will have a reasonable interface. It could be `SourceRange findDynamicExceptionSpecification(ArrayRef<Token> Tokens)`, for example. ================ Comment at: clang-tidy/modernize/UseNoexceptCheck.cpp:121 @@ +120,3 @@ + FileMoveRange.getEnd()), + Replacement); +} ---------------- I think, the problem is in the way you assign MoveRange in the checkHelper method. It is created as a character range and then you just assign its begin and end location. Instead, try `MoveRange = CharSourceRange::getTokenRange(I->getLocation(), Loc)`. ================ Comment at: clang-tidy/modernize/UseNoexceptCheck.h:44-45 @@ +43,4 @@ + unsigned &Len); + const std::string ReplacementStr; + StringRef Replacement; +}; ---------------- hintonda wrote: > aaron.ballman wrote: > > What is the difference between these two fields? The names are kind of > > confusing to me given their similarity, so perhaps renaming one to be more > > descriptive would be useful. > The first is the option passed in, i.e., the replacement string we will use > (either noexcept or a user macro like libcxx's _NOEXCEPT) for throw() and the > second one is the string we'll actually use for a particular replacement, > which can be what the user supplied, or noexcept(false) when the function can > throw. > > This was originally done with an auto in check(), but Alex wanted me to add a > helper function to make check() easier to understand. So, I had to put it > somewhere. I actually prefer the simpler original version. > > I'll see if I can come up with better names, but suggestions are always > welcome. The `StringRef Replacement;` should not be a member, since it's just one of the results of the `checkHelper` method. http://reviews.llvm.org/D18575 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
