alexfh added inline comments.
================
Comment at: clang-tidy/modernize-use-auto-min-type-name-length.cpp:61-83
+ long int li = static_cast<long int>(foo<long int>());
+ // CHECK-FIXES-0-0: auto li = {{.*}}
+ // CHECK-FIXES-0-5: auto li = {{.*}}
+ // CHECK-FIXES-1-0: auto li = {{.*}}
+ // CHECK-FIXES-1-5: auto li = {{.*}}
+ long int *pli = static_cast<long int *>(foo<long int*>());
+ // CHECK-FIXES-0-0: auto *pli = {{.*}}
----------------
zinovy.nis wrote:
> zinovy.nis wrote:
> > zinovy.nis wrote:
> > > alexfh wrote:
> > > > These tests could be more useful, if they verified boundary values of
> > > > the MinTypeNameLength, e.g. that `long int` is replaced with `auto`
> > > > when the option is set to 8 and it stays `long int`with the option set
> > > > to 9.
> > > >
> > > > Please also add tests with template typenames: e.g. the lenght of `T <
> > > > int >` should be considered 6 and the length of `T < const int >` is
> > > > 12. I guess, the algorithm I proposed will be incorrect for pointer
> > > > type arguments of templates (the length of `T<int*>` should be 7
> > > > regardless of `RemoveStars`), but this can be fixed by conditionally
> > > > (on RemoveStars) trimming `*`s from the right of the type name and then
> > > > treating them as punctuation. So instead of
> > > >
> > > > ```
> > > > for (const unsigned char C : tooling::fixit::getText(SR, Context)) {
> > > > const CharType NextChar =
> > > > std::isalnum(C)
> > > > ? Alpha
> > > > : (std::isspace(C) || (!RemoveStars && C == '*')) ? Space
> > > > :
> > > > Punctuation;
> > > > ```
> > > >
> > > > you could use something similar to
> > > >
> > > > ```
> > > > StringRef Text = tooling::fixit::getText(SR, Context);
> > > > if (RemoveStars)
> > > > Text = Text.rtrim(" \t\v\n\r*");
> > > > for (const unsigned char C : Text) {
> > > > const CharType NextChar =
> > > > std::isalnum(C) ? Alpha : std::isspace(C) ? Space : Punctuation;
> > > > ```
> > > > These tests could be more useful, if they verified boundary values of
> > > > the MinTypeNameLength, e.g. that long int is replaced with auto when
> > > > the option is set to 8 and it stays long intwith the option set to 9.
> > > >
> > >
> > > `int`-test is just for that :-)
> > >
> > > ```
> > > int a = static_cast<int>(foo()); // ---> int a = ...
> > > ```
> > I measured lengths for template cases:
> >
> >
> > ```
> > S=std::string * len=12
> > S=std::vector<std::string> * len=25
> > S=std::vector<const std::string> * len=31
> > S=std::string * len=12
> > S=std::vector<std::string> * len=25
> > S=std::vector<const std::string> * len=31
> > ```
> >
> >
> RemoveStars==1 here.
> S=std::string * len=12
With RemoveStars this should be 11?
> S=std::vector<std::string> * len=25
24?
> S=std::vector<const std::string> * len=31
30?
The point of my comment was that stars should be treated specially only at the
end of the type, not inside template parameters, e.g. `T<int****>` should have
length 10 regardless of RemoveStars. All your examples are with trailing stars.
================
Comment at: clang-tidy/modernize-use-auto-min-type-name-length.cpp:61-83
+ long int li = static_cast<long int>(foo<long int>());
+ // CHECK-FIXES-0-0: auto li = {{.*}}
+ // CHECK-FIXES-0-5: auto li = {{.*}}
+ // CHECK-FIXES-1-0: auto li = {{.*}}
+ // CHECK-FIXES-1-5: auto li = {{.*}}
+ long int *pli = static_cast<long int *>(foo<long int*>());
+ // CHECK-FIXES-0-0: auto *pli = {{.*}}
----------------
alexfh wrote:
> zinovy.nis wrote:
> > zinovy.nis wrote:
> > > zinovy.nis wrote:
> > > > alexfh wrote:
> > > > > These tests could be more useful, if they verified boundary values of
> > > > > the MinTypeNameLength, e.g. that `long int` is replaced with `auto`
> > > > > when the option is set to 8 and it stays `long int`with the option
> > > > > set to 9.
> > > > >
> > > > > Please also add tests with template typenames: e.g. the lenght of `T
> > > > > < int >` should be considered 6 and the length of `T < const int
> > > > > >` is 12. I guess, the algorithm I proposed will be incorrect for
> > > > > pointer type arguments of templates (the length of `T<int*>` should
> > > > > be 7 regardless of `RemoveStars`), but this can be fixed by
> > > > > conditionally (on RemoveStars) trimming `*`s from the right of the
> > > > > type name and then treating them as punctuation. So instead of
> > > > >
> > > > > ```
> > > > > for (const unsigned char C : tooling::fixit::getText(SR, Context)) {
> > > > > const CharType NextChar =
> > > > > std::isalnum(C)
> > > > > ? Alpha
> > > > > : (std::isspace(C) || (!RemoveStars && C == '*')) ? Space
> > > > > :
> > > > > Punctuation;
> > > > > ```
> > > > >
> > > > > you could use something similar to
> > > > >
> > > > > ```
> > > > > StringRef Text = tooling::fixit::getText(SR, Context);
> > > > > if (RemoveStars)
> > > > > Text = Text.rtrim(" \t\v\n\r*");
> > > > > for (const unsigned char C : Text) {
> > > > > const CharType NextChar =
> > > > > std::isalnum(C) ? Alpha : std::isspace(C) ? Space :
> > > > > Punctuation;
> > > > > ```
> > > > > These tests could be more useful, if they verified boundary values of
> > > > > the MinTypeNameLength, e.g. that long int is replaced with auto when
> > > > > the option is set to 8 and it stays long intwith the option set to 9.
> > > > >
> > > >
> > > > `int`-test is just for that :-)
> > > >
> > > > ```
> > > > int a = static_cast<int>(foo()); // ---> int a = ...
> > > > ```
> > > I measured lengths for template cases:
> > >
> > >
> > > ```
> > > S=std::string * len=12
> > > S=std::vector<std::string> * len=25
> > > S=std::vector<const std::string> * len=31
> > > S=std::string * len=12
> > > S=std::vector<std::string> * len=25
> > > S=std::vector<const std::string> * len=31
> > > ```
> > >
> > >
> > RemoveStars==1 here.
> > S=std::string * len=12
>
> With RemoveStars this should be 11?
>
> > S=std::vector<std::string> * len=25
>
> 24?
>
> > S=std::vector<const std::string> * len=31
>
> 30?
>
> The point of my comment was that stars should be treated specially only at
> the end of the type, not inside template parameters, e.g. `T<int****>` should
> have length 10 regardless of RemoveStars. All your examples are with trailing
> stars.
> int-test is just for that :-)
Nope, the point is to verify that the length of multi-token type names is
calculated correctly. `int` is a single token.
https://reviews.llvm.org/D45927
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
