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

Cleanup #1399

Draft
wants to merge 22 commits into
base: main
Choose a base branch
from
Draft

Cleanup #1399

wants to merge 22 commits into from

Conversation

Bike
Copy link
Member

@Bike Bike commented Nov 30, 2022

Mostly enables a lot more warnings in Clang.

Draft until I see any warnings on Mac and/or Cando are fixed.

@Bike Bike force-pushed the cleanup branch 2 times, most recently from 42b4644 to 74e4508 Compare December 2, 2022 18:58
Bike and others added 22 commits February 1, 2023 10:31
Some of the stuff in the reader had to be changed since it used
%s for characters, seemingly.
which means disabling a bunch of individual warnings, until I
fix them up.
I don't actually totally understand how these are pessimizing;
the basic case for this warning seems to be about copy elision in
return values. My best guess is that it has something to do with
the fact that takeError is defined to take ownership anyway, so
I guess it is kind of a move operation already, and maybe returns
a prvalue?
I'm not totally sure how ExecuteAction works, but I'm almost
entirely sure it's not supposed to hang. I copied the Base:: use in
the other ExecuteAction definition.
Some of these I'm not sure about, especially in the debugger. I
don't think there's really a good way to write a bulletproof
accessible_memory_p.
The order of C++ initializer lists is not used - fields are
initialized in the order they are declared in the class. This
matters when an initializer relies on another initialization in the
same initializer list having been completed, but in general we
probably ought to keep the orders the same to avoid any confusion.
The single dispatch miss stuff is not ready, and also I'm not
entirely sure it needs its own lisp function.
This code makes fmt complain because we pass it a weird enum, but
really the error message was unclear anyway.
On Macs, apparently, the underlying type of an enum is not int by
default, so fmt complains if you use %d.
The stream stuff I fixed up a bit since that's an ext: function
and we should not be presenting users with internal codes.
@Bike Bike marked this pull request as ready for review February 1, 2023 21:59
@Bike
Copy link
Member Author

Bike commented Feb 1, 2023

Still a lot of warnings. Some macro redefinitions and a lot of reorder-ctor in cando.

@Bike Bike marked this pull request as draft February 1, 2023 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants