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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
