- User Since
- May 26 2014, 12:49 PM (156 w, 2 d)
Raise no-hash logic up out of the hasher.
Minor improvement. Store the temporary storage in the type hasher so that we don't re-allocate on every record, but only when a record is larger than any other record we've seen.
I think you should be able to write a test for this too using a similar technique as mentioned in the previous review. Just create a yaml file with a module that contains a couple of source files. In theory you can omit any field you don't care about.
Can you add a test for this? It's really easy to generate yaml -> pdb tests now, just write a yaml file with everything cut out except for the fields you care about. It should pick up default values for everything else. Take a look at test\DebugInfo\PDB\Inputs\one-symbol.yaml for an example.
This seems ok to me, especially since there is no test coverage for it on Windows. rnk@, any thoughts?
Mon, May 22
Sat, May 20
You don't have to create Phabricator reviews for reverts. If something is broken on the bots, just push the revert. When you do, however, two things to keep in mind.
Fri, May 19
lgtm after reflowing the comment.
Thu, May 18
What would be the advantage of having it be a separate tool? I kind of like the fact that I don't have to remember which tool to use when I want something, which kind of solves the problem of tools with overlapping responsibilities. For example, I can never remember whether to use llvm-objdump or llvm-readobj. Seems to me they should be the same tool. Like "if you want to do something to a PDB, use llvm-pdbdump". That said, I do agree it does more than just dumping now, so perhaps a new name is in order soon. I wouldn't want to mix a change like that in with other changes though.
Added a slightly more complicated test case involving two identical procedure types.
Wed, May 17
Moved the folder. I really would like to put it in utils\unittest\googletest\include\gtest\Support, because then you could write
Removed the /DEBUGPDB option since it is unreferenced in the code. If the intent is to force writing a PDB, then all you have to do is specify /DEBUG, and you are guaranteed to get a PDB.
Yes, MSVC ignores /pdb option if you don't pass /DEBUG.
doh, I didn't actually submit this. I attached the wrong review to the commit message in Phabricator.
Tue, May 16
Mostly just that implicit conversion operators are a very subtle source of bugs, and you can't even find where they're being used because it's impossible to grep for it.
looks good with suggested fixes.
Deleted the forwarding constructors and made the private owning implementation names shorter.
Use enum instead of bool.
Also add the exception codes for printing a wide debugger string and setting a thread name.
Note that there is one instance of using CVTypeVisitor left that I couldn't get rid of in this patch. The one in RandomAccessVisitorTest. I have another patch which depends on this one which makes some fairly significantly change in this area, and after that patch we can get rid of that.
+amccarth since he's used some of this stuff before too.
Mon, May 15