Page MenuHomePhabricator

Names for structs are held on the Context, not the Module. Move getTypeByName from Module to Type taking a Context parameter.
AcceptedPublic

Authored by nicholas on Apr 23 2020, 11:06 PM.

Details

Summary

Creating a named struct requires only a Context and a name, but looking up a struct by name requires a Module. The method on Module merely accesses the LLVMContextImpl and no data from the module itself, so this patch moves getTypeByName to a static method on StructType that takes a Context and a name.

There's a small number of users of this function, they are all updated.

This updates the C API adding a new method LLVMGetTypeByName2, naming suggestions welcome. I considered LLVMGetTypeByNameInContext, but the LLVM*InContext pattern is used to differentiate between methods that work on the global context and those that don't, and that isn't the case here.

Diff Detail

Event Timeline

nicholas created this revision.Apr 23 2020, 11:06 PM
nicholas retitled this revision from Names for structs held are on the Context, not the Module. Move getTypeByName off Module. to Names for structs are held on the Context, not the Module. Move getTypeByName from Module to Type taking a Context parameter..Apr 28 2020, 9:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 28 2020, 9:50 AM

I think the new location is reasonable, I don't know about our C API policy though. Two questions below.

llvm/include/llvm-c/Core.h
874
  1. Should we use the deprecated attribute here?
  2. We cannot use attribute((overrload)) can we?
nickwasmer added inline comments.
llvm/include/llvm-c/Core.h
874
  1. There aren't any other uses of attribute deprecated, or any GNU-style attribute in the C API header files. At a guess, this might be for compatibility with non-GNU compilers.
  1. We can't. It would change the symbol name of both of them, which would be an ABI break for the existing LLVMGetTypeByName.
jdoerfert added inline comments.Apr 30 2020, 12:56 PM
llvm/include/llvm-c/Core.h
874
  1. Unclear if that is really a problem but OK.
  2. Makes sense.
CodaFi added a subscriber: CodaFi.Jun 9 2020, 12:39 PM

C API changes LGTM. My one request is to update the echo llvm-c-test to use the new API.

jdoerfert accepted this revision.Jun 10 2020, 1:41 PM

LLVM changes LGTM.

This revision is now accepted and ready to land.Jun 10 2020, 1:41 PM
nicholas updated this revision to Diff 272617.Jun 22 2020, 11:03 PM
nicholas added a project: Restricted Project.

Use LLVMGetTypeByName2 in llvm-c-test echo test too.

nicholas added inline comments.Jun 23 2020, 11:04 AM
llvm/include/llvm-c/Core.h
631

clang-tidy readability-identifier-naming doesn't like 'LLVMGetTypeByName2'. Should I ignore this because it's standard style for the LLVM C API? Or what should I change it to?