[lldb][TypeSystemClang][NFC] Make TemplateParameterInfos members private

[Michael137]

Summary

This patch makes the members of TemplateParameterInfos only accessible
via public APIs. The motivation for this is that
TemplateParameterInfos attempts to maintain two vectors in tandem
(args for the template arguments and names for the corresponding
name). Working with this structure as it's currently designed makes
it easy to run into out-of-bounds accesses later down the line.

This patch proposes to introduce a new
TemplateParameterInfos::InsertArg which is the only way to
set the TemplateArgument and name of an entry and since we
require both to be specified we maintain the vectors in sync
out-of-the-box.

To avoid adding non-const getters just for unit-tests a new
TemplateParameterInfosManipulatorForTests is introduced
that can be used to control internal state from tests.

[aprantl]

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

2067 ↗

(On Diff #482875)

I wish we had a lazy version of StringRef that computes the length only on demand. How long does template_param_infos live? If it's only temporary, maybe we could just make it store StringRefs?

[Michael137]

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

2067 ↗

(On Diff #482875)

I think that would be feasible. Though for this NFC change I'd prefer just keeping the changes to a minimum. There is this subtle distinction between a nullptr name and an empty string literal which affects how parameter packs are handled. I suspect there's ways to simplify those codepaths and then start using StringRefs instead. I'll look into it after this patch lands

[labath]

I think that test should just be rewritten to not modify the object in such a way. Instead of cumulatively modifying a single object, it could just create a fresh one each time. That will also make it clearer what is being tested. And I think it's fair game to add some constructors to the production object to make that easy.

For example, instead of

// long sequence of manipulator operations
ExpectNewTemplate("<typename T, long...> (T = int, long = 1)", infos);

it could do something like

ExpectNewTemplate("<typename T, long...> (T = int, long = 1)",
  TemplateParameterInfos({{"T", intType}}, /*pack_name=*/???, /*pack=*/TemplateParameterInfos(???));

[Michael137]

• No need for manipulator wrapper. Add constructors instead

[Michael137]

In D140030#3999199, @labath wrote:

I think that test should just be rewritten to not modify the object in such a way. Instead of cumulatively modifying a single object, it could just create a fresh one each time. That will also make it clearer what is being tested. And I think it's fair game to add some constructors to the production object to make that easy.

For example, instead of

// long sequence of manipulator operations
ExpectNewTemplate("<typename T, long...> (T = int, long = 1)", infos);

it could do something like

ExpectNewTemplate("<typename T, long...> (T = int, long = 1)",
  TemplateParameterInfos({{"T", intType}}, /*pack_name=*/???, /*pack=*/TemplateParameterInfos(???));

@labath good point, looks much better now. Even found a bug I introduced into the test :)

[Michael137]

• Fix tests

[labath]

That looks great. I have just one small nit. I think the test would read better if you made the names of temporary TemplateArgument objects more descriptive -- i.e. encode the kind and value information in the name. For example. int47Arg for an integer template argument whose value is 47, and intTypeArg for a type template argument (where int is the "value").

[Michael137]

Got landed in https://reviews.llvm.org/rGa29e06bbeaad7012ac85b221f1aaba3fe1d5fd35

Forgot about your last comment re. variable names in the tests Pavel. Will change in a follow-up commit

[Michael137]

Didn't rebase properly and broke the build
Fixing...

[Michael137]

Argh seems like Phabricator didn't like the fact I linked the revision in a different commit. So to see the actual change that got landed as part of this you need to visit the history tab and exclude the latest commit.

[Michael137]

Test variable renaming committed in https://reviews.llvm.org/rG8e8dd110be5f

lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp

 }
 
 std::string TypeSystemClang::PrintTemplateParams(
     const TemplateParameterInfos &template_param_infos) {
   llvm::SmallVector<NamedDecl *, 8> ignore;
   clang::TemplateParameterList *template_param_list =
       CreateTemplateParameterList(getASTContext(), template_param_infos,
                                   ignore);
-  llvm::SmallVector<clang::TemplateArgument, 2> args =
-      template_param_infos.args;
+  llvm::SmallVector<clang::TemplateArgument, 2> args(
+      template_param_infos.GetArgs());
   if (template_param_infos.hasParameterPack()) {
-    args.append(template_param_infos.packed_args->args);
+    llvm::ArrayRef<TemplateArgument> pack_args =
+        template_param_infos.GetParameterPackArgs();
+    args.append(pack_args.begin(), pack_args.end());
   }
   std::string str;
   llvm::raw_string_ostream os(str);
   clang::printTemplateArgumentList(os, args, GetTypePrintingPolicy(),
                                    template_param_list);
   return str;
 }