Skip to content

Conversation

@redDwarf03
Copy link
Contributor

No description provided.

@csoler csoler requested a review from G10h4ck April 25, 2025 19:29
Copy link
Contributor

@G10h4ck G10h4ck left a comment

Choose a reason for hiding this comment

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

I am really happy to see contributions in this directions. IMHO some parts like the changes to JSON API generator needs to be tested on as much platforms as possible (oldest non-EOL ubuntu LTS available, debian stable, gentoo, some other linux, and some older macOS too). Looks good overall, also look at my inline comments. An organic description of the PR would help too :)

pgp/pgpkeyutil.h
pgp/rscertificate.h )
pgp/rscertificate.h
pgp/rnppgphandler.h )
Copy link
Contributor

Choose a reason for hiding this comment

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

This changes seems good, and I think they are needed for the work @csoler merged a bit of time ago, do you confirm @csoler ?

pqi/pqisslpersongrp.h
pqi/pqisslproxy.h
pqi/pqissludp.h
pqi/pqissl.h
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit surprising, I would expect including pqissl.h to be needed to compile in the first place, but for some reason it was able to build anyway before


template <typename UpdateMap,class ItemClass>
struct get_second : public std::unary_function<typename UpdateMap::value_type, RsItem*>
struct get_second
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to fix compatibility with newer compilers, am I wrong?

except KeyError:
print(f"Warning: Parameter '{tmpPN.text}' found in Doxygen description for {refid}, but not in function signature parameters {list(paramsMap.keys())}. Skipping direction assignment.")
continue # Skip to the next parametername tag

Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIR we should fail in this case, in addition to print the error, so compilation fail and developers notice ASAP they screw-up something, am I wrong?

# Handle more complex cases or leave as is if unsure
# For now, just log a warning if we encounter an unexpected reference type
print(f"Warning: Encountered potentially complex reference type '{mp._type}' for parameter '{mp._name}'. Declaration might be incorrect.")
declaration_type = declaration_type.replace('&', '').strip() # Fallback: simple removal
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIR if something unexpected/unhanded is encountered compilation must fail, so developers notice ASAP they screw-up something.

*/
struct CompareCharIC :
public std::binary_function< char , char , bool> {
struct CompareCharIC {
Copy link
Contributor

Choose a reason for hiding this comment

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

Removing deprecated stuff looks OK here too.

} else {
LIBRNP_LIBS *= -lbotan-2
# This is the case for macOS and other Unix-like systems
LIBRNP_LIBS *= -lbotan-3
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this works on older non-EOL macOS ? AFAIR we have some qmake functions to check for specific macOS versions if botan-2 is still needed for some of them

@defnax
Copy link
Contributor

defnax commented Jun 8, 2025

@csoler @G10h4ck check this again?

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.

3 participants