- User Since
- May 26 2014, 12:49 PM (151 w, 5 d)
Fri, Apr 21
Fix an inverted conditional / break statement.
Updated based on feedback from chandlerc.
Thu, Apr 20
Wed, Apr 19
Reduced indentation level a little bit.
Deleted some miscellaneous functions in MathExtras.h that I ended up not using, and added some comments to the implementation to clarify a few minor points.
lgtm, I can commit it for you later today.
No need to request review when removing dead code.
Did you run clang-format on the CL? (It looks clang-formatted to me, just want to make sure)
Tue, Apr 18
Can you make the test run llvm-cvtres.exe /? and then have a FileCheck line that matches the first line of help text?
Fri, Apr 14
Thu, Apr 13
Wed, Apr 12
Tue, Apr 11
How much would it complicate things to move the hand maintained files out of tree? If the Xcode build isn't really a thing we're officially supporting, perhaps we can take that aggressive approach in-tree as well?
I guess what I mean is, if you just want to import every single top level option into the "run" subcommand, and the tool only works when the run subcommand is given (looking at the source code it just prints usage and exits if there is no run subcommand) why is the run subcommand even necessary? Isn't it equivalent to just not even having a subcommand and running llvm-lto2 -lto-use-new-pm ...?
So currently if you were to run llvm-lto2 -help would you already see all the subcommands you want to import? It sounds like yes. If so, what's the difference between llvm-lto2 -lto-use-new-pm and llvm-lto2 run -lto-use-new-pm? Is there some reason you need to import them all rather than just use the existing top-level options?
Actually perhaps better would be to make cl::redefine just another modifier. Maybe call it cl::template_opt or something. This way you would write:
So if I understand correctly, these are existing options that are defined in some library somewhere, and you want to be able to use them in a subcommand of your own creation? Yea unfortunately I never considered that use case.
Networking isn't my area of domain expertise, so these are mostly just general comments.
Mon, Apr 10
Updated this so that isa<B>(const unique_ptr<A>&) works. Will commit tomorrow afternoon if nobody has objections.
Discussed this with rsmith@ some, and we came to the conclusion that it's actually the other way around. We should support this only for cast, and not for dyn_cast. The reason being that you would need to declare the function as dyn_cast(unique_ptr<T> &&Value), and so to invoke it you would need to write dyn_cast<To>(std::move(From)). So even though you're writing this as an unconditional move, it's actually a conditional move, because if it's not actually convertible to To then you would want to return nullptr and leave the source unchanged. The code would still work, but it would just be confusing because std::move(From) sometimes doesn't actually move anything.
lgtm, only some minor nits.
Sun, Apr 9
Factored the return statement into a small helper function.
Sat, Apr 8
Fixed an issue where find_first_unset() was returning BV.size() if all bits were set, and added a unit test for that case.
Thu, Apr 6
Use a better method for detecting padding. This is less error prone and more efficient. It also allows us to analyze the padding one time up front, which allows us to filter record display based on whether or not the record has padding (e.g. only dump those records that have padding).
Slightly better logic in layout mode. It was dumping constants and static members before which don't occupy any space in a record.
Not really sure who to add as a reviewer here, so + a few random people.
Tue, Apr 4
Mon, Apr 3
The 8KB thing is interesting, because sometimes it's not actually every 8KB. I don't know how important it is though, since it should just change the efficiency of the log(N) search. Run llvm-pdbdump analyze on a large PDB and then paste the results into Spreadsheets and compute differences to see what I mean.
Mar 22 2017
I think we need something before you commit, because as it is people will be specifying --color-output unnecessarily. Even if you say "default = on" or "default = best-effort"
It's hard for me to review on correctness since I'm not familiar with these APIs (and I don't think anyone else is either) but from a functionality standpoint I think these are great changes and I don't see anything glaringly wrong, so lgtm unless someone else has objections.