Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

APInt API is atrocious #5579

Closed
lattner opened this issue Oct 16, 2009 · 7 comments
Closed

APInt API is atrocious #5579

lattner opened this issue Oct 16, 2009 · 7 comments
Labels
bugzilla Issues migrated from bugzilla code-cleanup

Comments

@lattner
Copy link
Collaborator

lattner commented Oct 16, 2009

Bugzilla Link 5207
Resolution FIXED
Resolved on Dec 09, 2010 02:35
Version 1.0
OS All
CC @asl,@jayfoad

Extended Description

The APInt API is extremely inconsistent. Among other things, some methods update the APInt in place, and some return a new APInt. This leads to awful stuff like:

V = V.lshr(ByteStart*8);
V.trunc(ByteSize*8);

I think that we should fix this by changing operations that update in place to have an _eq suffix, this would give us:

V = V.lshr_eq(ByteStart*8);
V = V.trunc_eq(ByteSize*8);

or, better yet in this case:

V.lshr(ByteStart*8);
V.trunc(ByteSize*8);

The ones that update in place should return void to make it really clear that they do not return a new APInt.

@lattner
Copy link
Collaborator Author

lattner commented Oct 16, 2009

Sorry, I got that completely backward. This is what we'd have (foo_eq corresponds to things like +=):

V = V.lshr(ByteStart*8);
V = V.trunc(ByteSize*8);

or, better yet in this case:

V.lshr_eq(ByteStart*8);
V.trunc_eq(ByteSize*8);

@llvmbot
Copy link
Collaborator

llvmbot commented Oct 17, 2009

+1

@jayfoad
Copy link
Contributor

jayfoad commented Nov 26, 2010

I think that we should fix this by changing operations that update in place to
have an _eq suffix

The public non-const non-static non-operator methods in question are:

  • Read(). Doesn't seem to be used or even implemented, and nor is Emit(), so I suggest removing them both. (I've sent a patch for this to llvm-commits, currently awaiting list moderator approval.)

  • trunc(), sext(), zext(), sextOrTrunc(), zextOrTrunc().

  • set(), clear(), flip(). Are you sure you want these to have an _eq suffix? It doesn't seem to read very nicely, especially the versions that take no argument.

  • doubleToBits(), floatToBits(). I suggest changing them to return a new APInt instead. (I've sent a patch for this to llvm-commits, currently awaiting list moderator approval.)

@lattner
Copy link
Collaborator Author

lattner commented Nov 28, 2010

Sounds great!

@jayfoad
Copy link
Contributor

jayfoad commented Dec 7, 2010

  • Read(). Doesn't seem to be used or even implemented, and nor is Emit(), so I
    suggest removing them both.

Done:
http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20101122/112578.html

  • trunc(), sext(), zext(), sextOrTrunc(), zextOrTrunc().

Changed them to return a new value:
http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20101206/113298.html
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20101206/037106.html
http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20101206/113299.html

  • set(), clear(), flip(). Are you sure you want these to have an _eq suffix? It
    doesn't seem to read very nicely, especially the versions that take no
    argument.

Made them return void:
http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20101129/112774.html

... and renamed them:
http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20101129/112909.html

  • doubleToBits(), floatToBits(). I suggest changing them to return a new APInt
    instead.

Done:
http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20101122/112577.html

Is there anything else wrong with the APInt API, or can we close the bug?

@lattner
Copy link
Collaborator Author

lattner commented Dec 9, 2010

It's looking great to me new Jay, please do close this. Thanks again for tackling this!

@jayfoad
Copy link
Contributor

jayfoad commented Dec 9, 2010

Fixed, then!

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 3, 2021
Michael137 pushed a commit to Michael137/llvm-project that referenced this issue Dec 2, 2022
…tch-nested-generics

Add nested generic LLDB metadata tests
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugzilla Issues migrated from bugzilla code-cleanup
Projects
None yet
Development

No branches or pull requests

3 participants