I think this looks good! Probably we still want someone more experienced than me to say something more insightful... (I am primarily experienced in browsing static code checking results, not setting them up...)

Copy link

MemberAuthor

tacaswell commentedSep 7, 2022

I would also like feedback from@QuLogic or@anntzer about the c++ changes to deal with the overflows (I did it two ways, is one better than the other? Is it important to be consistent here? should we have just pushed everything to bigger types?).

Copy link

Contributor

anntzer commentedSep 7, 2022•
edited
Loading

It would probably be better to be consistent and use ssize_t everywhere, but let's not lose sleep over that for now.

Also, AFAICT the whole to_string_argb is completely unused (and can be easily implemented with numpy, it's the similar to one-line byteorder swap in cbook._premultiplied_argb32_to_unmultiplied_rgba8888 without the extra alpha handling), so I would just deprecate it together with the Python-level wrappers.

github-actionsbot added the status: needs rebase label

Oct 19, 2022

tacaswell force-pushed theci_add_codeql branch froma202190 to19daeffCompare

October 20, 2022 22:03

github-actionsbot removed the status: needs rebase label

Oct 20, 2022

tacaswell marked this pull request as draft

October 20, 2022 22:06

Copy link

MemberAuthor

tacaswell commentedOct 20, 2022

Back to draft because the build does not work again...

anntzer reviewed

Oct 21, 2022

View reviewed changes

src/_backend_agg.h

		inlinesize_tnum_paths()const
		{
		return m_meshWidth * m_meshHeight;
		return(size_t)m_meshWidth * m_meshHeight;

Copy link

Contributor

anntzerOct 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

py_adaptors.h defines num_paths as Py_ssize_t, perhaps better for consistency, though anything will work, of course.

Copy link

Contributor

anntzer commentedOct 21, 2022

Nearly all the changes just hit the now deprecated to_string{,_argb}; just left an additional point re: signedness of num_paths, though it's not a big deal either way.

QuLogic force-pushed theci_add_codeql branch from28d323b to9de072bCompare

November 26, 2022 01:52

Copy link

Member

QuLogic commentedNov 26, 2022

OK, this is now passing, though I will likely want to clean up the YAML. The JavaScript analysis takes ~6m; the C++/Python one takes ~17m. I think I can split C++/Python back up again now that it's working so that it'll finish quicker. Hopefully we don't end up with too many workflows running in parallel.

QuLogic force-pushed theci_add_codeql branch 4 times, most recently frome3e83db to7430026Compare

November 26, 2022 06:26

CI: Add CodeQL analysis

41833f5

Co-authored-by: Oscar Gustafsson <oscar.gustafsson@gmail.com>Co-authored-by: Thomas A Caswell <tcaswell@gmail.com>

QuLogic force-pushed theci_add_codeql branch from7430026 to41833f5Compare

November 26, 2022 07:06

tacaswelland others added2 commits

November 26, 2022 02:43

MNT: ensure pre-multiplication casting to prevent overflow

64fc13a

BLD: Add an explicit message if certifi is unavailable

340eee2

If you only do a build step (i.e., `python setup.py build`), it does notgo through all the build dependency machinery, and then mysteriouslysays it 'cannot download' a URL, but really it's a missing dependency.

Copy link

Member

QuLogic commentedNov 26, 2022

I squashed down everything, removed the C++ change and confirmed that CodeQL is still warning about it, so I've now added it back in along with a more explicit message about why the build broke earlier.