Thanks for the comments. Added some replies.

In D78796#2026009, @dsanders wrote:

In D78796#2025835, @nlguillemot wrote:

In D78796#2025650, @dsanders wrote:

I'm inclined to agree that the patch series as-is doesn't really warrant the iterators as the interface as no callers have been updated. However, I also don't see much that's iterator specific (ULEBifier would be roughly similar code leaving the iterator portion as trivial wrappers on the ULEBifier) and there are a few places (particularly in tablegen) that are emitting LEB's into containers where the loop to add bytes one by one is just noise and something like std::copy(to_uleb(...), std::back_inserter(Table)); would be somewhat nice. The loop could easily be hidden in something like append_uleb(Table, ...) though I don't think there's a strong argument for (or against) iterators.

What I would suggest is separating out the byte-sequence generation into a ULEBifier as David suggested but still keeping the iterator object as a thin adapter (effectively implementing that while loop) to support inter-operation with STL functions like std::copy.

What is the core issue with the iterator interface that makes it desirable to have something like ULEBifier<T> instead?

For me it's that iterators reference and indirect into the elements of a container. They shouldn't be the container themselves

In D78796#2025650, @dsanders wrote:

I'm inclined to agree that the patch series as-is doesn't really warrant the iterators as the interface as no callers have been updated. However, I also don't see much that's iterator specific (ULEBifier would be roughly similar code leaving the iterator portion as trivial wrappers on the ULEBifier) and there are a few places (particularly in tablegen) that are emitting LEB's into containers where the loop to add bytes one by one is just noise and something like std::copy(to_uleb(...), std::back_inserter(Table)); would be somewhat nice. The loop could easily be hidden in something like append_uleb(Table, ...) though I don't think there's a strong argument for (or against) iterators.

What I would suggest is separating out the byte-sequence generation into a ULEBifier as David suggested but still keeping the iterator object as a thin adapter (effectively implementing that while loop) to support inter-operation with STL functions like std::copy.

In D78796#2007033, @dblaikie wrote:

I'm not sure this is worth a full iterator abstraction - would the uses be that much the worse for it if they had a non-iterator type they had to iterate manually & pull values from? A more simplified iterator abstraction, essentially:

ULEBifier U(Value, IsSigned, PadTo);
while (Optional<char> C = U.next())
  OS.write(*C);

Or something like that.

Thanks for the review @dblaikie !

In D78795#2007027, @dblaikie wrote:

Maybe hold of on committing this until some of the rest of the patch sequence has been reviewed/approved, in case it doesn't end up being needed.

• Added some basic tests that use raw_ostream_iterator manually instead of through std::copy.
• Replaced manual calls to flush() in the test with an artificial scope (takes advantage of the fact that ~raw_string_ostream() calls flush())
• Added a test for std::copy with an empty range.

Abandoning this project.

Replaced assignments of 0 with assignments of ValueT(), as a more generic way to get a zero value.

Updated comments about zext to include a mention of sext.

Rebased this patch on top of its parent commit.

Removed unnecessary inline

Refactored the encode functions more aggressively by factoring out the common logic for writing to an array or to a raw_ostream:

diff
diff --git a/llvm/include/llvm/Support/LEB128.h b/llvm/include/llvm/Support/LEB128.h
index 729ee5ca745..f65dc919e39 100644
--- a/llvm/include/llvm/Support/LEB128.h
+++ b/llvm/include/llvm/Support/LEB128.h
@@ -168,43 +168,52 @@ public:
   ///@}
 };

Added a unit test that uses the output iterator in a std algorithm, just as a sanity test that the class can indeed be used as an output iterator. The code in decode[U|S]LEB128 doesn't use the output iterator with std algorithm, so this extra test was added to get coverage for that use case.

Forgot to include a few changes in the original diff that was posted.

The end goal of this series of patches is to support encoding/decoding APInt to/from [U|S]LEB128. This patch is an initial step in that direction. It refactors the logic to avoid code duplication and makes the interface to LEB128 decoding more generic.

This is patch is mainly useful for enabling some of the refactors that happen in the next patch here: https://reviews.llvm.org/D78796

The end goal of this series of patches is to support encoding/decoding APInt to/from [U|S]LEB128. This patch is an initial step in that direction. It refactors the logic to avoid code duplication and makes the interface to LEB128 encoding much more generic, as shown by the existing cases becoming very short.

Some example code and the commit message were showing an example of printing a vector<double>. It turns out that doubles don't print so beautifully by default, so switch the example to use int instead to avoid being misleading.

In D68054#1801066, @thopre wrote:

How about the following commit message:

(...)

In D68054#1755059, @thopre wrote:

Not committing to remove the ability to call match/sub on an invalid regex implies that the addition of the new API needs to be justified independently of the extra check that needs to be performed in match/sub to allow the current behaviour. And in fact, the only motivation for this patch I could think of was consistency with other languages and I was finding it a hard sell and was about to apologise for the extra work I made you do.

However I overlooked one benefit of removing the ability to match an invalid regex: robustness. All the cases where a call to match/sub is guards a code other than throwing an error is likely to result in wrong behaviour in case the regex is invalid. So I think it would be safer to use a separate API for combined regex creation + match/sub as per the previous revision of this patch and prevent the existing approach, so that users are mindful when not doing separate checks.

What do you think? If you agree that would mean reverting to the previous version of this patch and the simultaneous apologies from me for requesting the current version.

In D68155#1751826, @thopre wrote:

My sincere apologies, I forgot to change the author in the commit. I don't often commit on behalf of others so didn't think enough.

In D68054#1751415, @thopre wrote:

In D68054#1750788, @nlguillemot wrote:

Removed the assert inside match() that made it crash when match() is called with a regex pattern that isn't successfully compiled. Also removed the "death test" unit tests that tested that this assert was triggering.

Can you reword the description along the line of this new API being a convenience when one does not care about distinguishing validity of a regex from whether it matches?

Removed the assert inside match() that made it crash when match() is called with a regex pattern that isn't successfully compiled. Also removed the "death test" unit tests that tested that this assert was triggering.

Opened a review that just increases the const-correctness of Regex in clang: https://reviews.llvm.org/D68155

In D68054#1685676, @thopre wrote:

The following clang unit tests fail with your patch:

Format/./FormatTests/FormatTest.FunctionAnnotations
Format/./FormatTests/FormatTest.UnderstandsFunctionRefQualification

Can you have a look?

• Added more comments to static match and static sub to clarify the return value and the error's value.
• Remove updates of "static Regex" -> "static const Regex", to do them in a future separate patch instead.
• Switch order of test lines in "ConvenienceFunctions" test.

In D67241#1679847, @thopre wrote:

In D67241#1679348, @nlguillemot wrote:

friendly reminder ping

My bad, I misunderstood you and thought you'd do a patch to implement the "Create a Regex and immediately throw it away" idiom and rebase that one on top of it.

friendly reminder ping

Refactored loop over the list of vectors to avoid the tricky array/pointer loops.

Added note in the comments for match and sub to explicitly note that the Error string is cleared when there is no error.

I don't have commit privileges yet. Could somebody commit this for me?

Could somebody do the commit for me? I don’t have commit access yet.

ping

ping

ping

In D63320#1549309, @craig.topper wrote:

Can the enum be used without this? I'm trying to understand why the enum was there to begin with. Only one in tree target, AVR, defines GET_INSTRINFO_OPERAND_TYPES_ENUM. Its not built by default so I could't really check, but looking through the file that included it, I couldn't prove it was being used.

some syntax nits:
OpcodeOperandTypes [] -> OpcodeOperandTypes[] (removed unnecessary space)
OS << "-1"; -> OS << -1; (don't use string where not necessary

• Fix some syntax mistakes.
• Map only records that derive from Operand and are not anonymous. This exactly matches the condition used to generate GET_INSTRINFO_OPERAND_TYPES_ENUM.