Page MenuHomePhabricator

Make parseBitcodeFile use a named StructType, if it exists and matches.
Needs ReviewPublic

Authored by arnt on Fri, Apr 12, 7:23 AM.

Details

Summary

Until now, parseBitcodeFile() would use an already-existing StructType if
it EITHER had the right structure and no name, OR the right name and no
structure, but not if it had both the right name and the right structure.
This change makes parseBitcodeFile() check the struct and use it if
appropriate.

It still creates a new StructType with a new name if the existing type and
what it needs to read differs in structure.

It also add a new function to retrieve a named StructType by name. The new
function is a lookup-only function; it doesn't change anything at all.

Diff Detail

Event Timeline

arnt created this revision.Fri, Apr 12, 7:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptFri, Apr 12, 7:23 AM
arnt added a comment.Fri, Apr 12, 7:26 AM

I added lebedev.ri and the last two people to touch the file as reviewers, I hope that's not too much of an imposition.

dnsampaio resigned from this revision.Fri, Apr 12, 8:19 AM

Sorry, I don't believe I ever touched these files before.

lebedev.ri added inline comments.Fri, Apr 12, 8:28 AM
llvm/lib/Bitcode/Reader/BitcodeReader.cpp
1548

Pass by non-const value

1549

ArrayRef

1550

I'm not sure this is clang-formatted

1555–1558
if(!EltTys.equals(Candidate->elements()))
  return nullptr;
llvm/lib/IR/Type.cpp
342

nullptr

348

sporadic newline

arnt updated this revision to Diff 194907.Fri, Apr 12, 9:37 AM
arnt marked an inline comment as done.

more LLVM-y style, and better style too. Modern.

arnt marked an inline comment as done and an inline comment as not done.Fri, Apr 12, 9:42 AM
arnt added a subscriber: dnsampaio.

@dnsampaio Sorry about that. I looked at my shell history now, and I think I added you because I got a wildcard wrong and picked the most recent two committers for the wrong set of files.

llvm/lib/Bitcode/Reader/BitcodeReader.cpp
1555–1558

Oh, that's neat. I hadn't noticed that being done elsewhere, but I like it better. I'll push as soon as the unit tests have run.

(not all inline remarks were addressed)

llvm/include/llvm/IR/DerivedTypes.h
271–276

This probably deserves a /// doxygen comment

llvm/lib/Bitcode/Reader/BitcodeReader.cpp
1549–1550

here and elsewhere - not from clang-format
(you can just setup a git pre-commit hook so all your commits will have correct formatting)

arnt marked 4 inline comments as done.Mon, Apr 15, 7:13 AM

I didn't mean to suggest that I'd done all of them, I just ran out of working time on Friday. I attended to the rest now and will push a new revision as soon as the tests have run.

llvm/include/llvm/IR/DerivedTypes.h
271–276

Yes.

There could be a lot more of them in general. Doxygen sucks.

llvm/lib/Bitcode/Reader/BitcodeReader.cpp
1549–1550

I added the emacs integration. I'll try the git commit hook before my next pull request, thanks for the suggestion.

arnt updated this revision to Diff 195180.Mon, Apr 15, 7:39 AM
arnt marked 2 inline comments as done.

Attended to the rest of Lebedev.ri's comments, plus clang-format

For reference, there is a trivial way to correctly format all the changed code:
https://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting

llvm/unittests/Bitcode/BitReaderTest.cpp
217

EXPECT_EQ() / EXPECT_ME() ?

arnt marked an inline comment as done.Sun, Apr 21, 12:46 PM

Thanks.

FWIW I noticed that the author of the git-commit hook suggested using an emacs-lisp clang-format wrapper, and decided to try that first, until I grow unhappy.

llvm/unittests/Bitcode/BitReaderTest.cpp
217

Yes, OK, will fix, although YAGNI and so on.

arnt updated this revision to Diff 196206.Tue, Apr 23, 3:53 AM

This revision uses more appropriate gtest matchers.