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

[Siva Chandra via Phabricator via libc-commits]

sivachandra accepted this revision.
sivachandra added inline comments.


================
Comment at: libc/test/src/__support/vlist_holder_test.cpp:15
+  va_list vlist;
+  va_start(vlist, n);
+  __llvm_libc::internal::VListHolder v(vlist);
----------------
michaelrj wrote:
> 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.
OK. I am not very confident about how `va_list` is implemented. For example, calling `va_end` in a destructor is also not OK as it is a different stack frame. OK for now.


================
Comment at: libc/test/src/__support/vlist_holder_test.cpp:20
+  for (int i = 0; i < n; ++i) {
+    v.next_var<int>();
+  }
----------------
michaelrj wrote:
> 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.
Ah, yes! Sorry for the noise.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123061