[libc-commits] [PATCH] D123061: [libc] Add holder class for va_lists

[Michael Jones via Phabricator via libc-commits]

michaelrj added inline comments.


================
Comment at: libc/src/__support/vlist_holder.h:17
+
+class VListHolder {
+  va_list vlist;
----------------
sivachandra wrote:
> lntue wrote:
> > Maybe VaListHolder is a bit clearer?
> I vote for something even simpler: `ArgList`?
I went with `ArgList` since it's shorter.


================
Comment at: libc/test/src/__support/vlist_holder_test.cpp:15
+  va_list vlist;
+  va_start(vlist, n);
+  __llvm_libc::internal::VListHolder v(vlist);
----------------
sivachandra wrote:
> Should the `va_start` and `va_end` be managed by the new class?
The class handles its own `va_end`, but I don't think it can handle the `va_start` since its constructor is a new function and you can only create a `va_list` for the scope of the current function. I could probably create a macro to handle this invisibly, but that's as close as we can get, I think.


================
Comment at: libc/test/src/__support/vlist_holder_test.cpp:20
+  for (int i = 0; i < n; ++i) {
+    v.next_var<int>();
+  }
----------------
sivachandra wrote:
> Implementing the c++ iterator protocol will help will make the for-loop iteration cleaner?
I don't think an iterator works here, since the iterator needs to have a type, and the values may be of various types. In addition, there's no end marker or length in a `va_list`, meaning that a for loop won't know when to end.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123061