[PATCH] D116290: [clang-format] Add enforcement of consistent `class`/typename` keyword for template arguments

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added inline comments.


================
Comment at: clang/lib/Format/TemplateArgumentKeywordFixer.cpp:55
+    // For `auto` language version, be conservative and assume we are < C++17
+    KeepTemplateTemplateKW = (Style.Standard == FormatStyle::LS_Auto) ||
+                             (Style.Standard < FormatStyle::LS_Cpp17);
----------------
Quuxplusone wrote:
> curdeius wrote:
> > avogelsgesang wrote:
> > > curdeius wrote:
> > > > Isn't it a better name?
> > > This flag is actually about the usage of the `class` keyword instead of the `typename` keyword for template-template arguments.
> > > `true` means: "Keep using the `class` instead of the `typename` keyword for template-template arguments."
> > > 
> > > I think the name `KeepTemplateTypenameKW` is wrong. "[...]TypenameKW = true" would mean "use `typename` instead of `class`" to me, and that's exactly the opposite way around.
> > > 
> > > As such, I think `KeepTemplateTemplateKW` is in fact the better name. If we want to make it even more explicit, we could also use `KeepTemplateTemplateClassKW`. What do you think?
> > I did understand it correctly that it is about class keyword in template template parameters, but my brain somehow melted down the road:). You can keep the name as is.
> IIUC, it feels like the boolean should be named `UseClassKWInTemplateTemplates`, and it shouldn't have anything to do with `Keep`ing the human's choice. If the human feeds you some C++17 code using `template<template<class> typename T>` and asks you to format it as C++14 with `TAS_Typename`, I think it would be quite appropriate to output `template<template<typename> class T>`. (Change `class` to `typename` because the human asked for `TAS_Typename`; change `typename` to `class` because C++14 implies `UseClassKWInTemplateTemplates`.)
> Anyway, this should have a unit test too.
> IIUC, it feels like the boolean should be named `UseClassKWInTemplateTemplates`, and it shouldn't have anything to do with `Keep`ing the human's choice. If the human feeds you some C++17 code using `template<template<class> typename T>` and asks you to format it as C++14 with `TAS_Typename`, I think it would be quite appropriate to output `template<template<typename> class T>`. (Change `class` to `typename` because the human asked for `TAS_Typename`; change `typename` to `class` because C++14 implies `UseClassKWInTemplateTemplates`.)
> Anyway, this should have a unit test too.




================
Comment at: clang/lib/Format/TemplateArgumentKeywordFixer.cpp:55
+    // For `auto` language version, be conservative and assume we are < C++17
+    KeepTemplateTemplateKW = (Style.Standard == FormatStyle::LS_Auto) ||
+                             (Style.Standard < FormatStyle::LS_Cpp17);
----------------
curdeius wrote:
> Quuxplusone wrote:
> > curdeius wrote:
> > > avogelsgesang wrote:
> > > > curdeius wrote:
> > > > > Isn't it a better name?
> > > > This flag is actually about the usage of the `class` keyword instead of the `typename` keyword for template-template arguments.
> > > > `true` means: "Keep using the `class` instead of the `typename` keyword for template-template arguments."
> > > > 
> > > > I think the name `KeepTemplateTypenameKW` is wrong. "[...]TypenameKW = true" would mean "use `typename` instead of `class`" to me, and that's exactly the opposite way around.
> > > > 
> > > > As such, I think `KeepTemplateTemplateKW` is in fact the better name. If we want to make it even more explicit, we could also use `KeepTemplateTemplateClassKW`. What do you think?
> > > I did understand it correctly that it is about class keyword in template template parameters, but my brain somehow melted down the road:). You can keep the name as is.
> > IIUC, it feels like the boolean should be named `UseClassKWInTemplateTemplates`, and it shouldn't have anything to do with `Keep`ing the human's choice. If the human feeds you some C++17 code using `template<template<class> typename T>` and asks you to format it as C++14 with `TAS_Typename`, I think it would be quite appropriate to output `template<template<typename> class T>`. (Change `class` to `typename` because the human asked for `TAS_Typename`; change `typename` to `class` because C++14 implies `UseClassKWInTemplateTemplates`.)
> > Anyway, this should have a unit test too.
> > IIUC, it feels like the boolean should be named `UseClassKWInTemplateTemplates`, and it shouldn't have anything to do with `Keep`ing the human's choice. If the human feeds you some C++17 code using `template<template<class> typename T>` and asks you to format it as C++14 with `TAS_Typename`, I think it would be quite appropriate to output `template<template<typename> class T>`. (Change `class` to `typename` because the human asked for `TAS_Typename`; change `typename` to `class` because C++14 implies `UseClassKWInTemplateTemplates`.)
> > Anyway, this should have a unit test too.
> 
> 
+1

I think that generally you should change only the cases that match some pattern (e.g. typename/class followed by a single identifier without `=` nor `::`, etc., similarly for template template parameters) instead of changing everything except some cases. This will be much less error-prone.
Also, please add tests as suggested above.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116290/new/

https://reviews.llvm.org/D116290