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

Doubts about some *const pointers #4301

Closed
alejandro-colomar opened this issue May 12, 2024 · 5 comments
Closed

Doubts about some *const pointers #4301

alejandro-colomar opened this issue May 12, 2024 · 5 comments
Labels

Comments

@alejandro-colomar
Copy link
Member

neomutt/email/envelope.h

Lines 70 to 71 in 20fe7bd

char *const subject; ///< Email's subject
char *const real_subj; ///< Offset of the real subject

What's the use of these *consts if you have to cast them away all the time?

@nabijaczleweli

@flatcap
Copy link
Member

flatcap commented May 12, 2024

The idea of b5640bb was to ensure that the subject and real_subj are kept in sync.
The const is a hint to call mutt_env_set_subject() rather than altering them directly.

@nabijaczleweli
Copy link
Member

nabijaczleweli commented May 12, 2024

FTR the fields are const, not the pointers; don't cast the const away, use the setter

@alejandro-colomar
Copy link
Member Author

alejandro-colomar commented May 12, 2024

FTR the fields are const, not the pointers;

I'm not sure what you mean by this. char *const p means "p is a constant pointer to char", so the pointer is const.

Maybe you meant the the pointees aren't const, which I agree.

https://cdecl.org/

don't cast the const away, use the setter

Except that I count 10 casts outside of the setter. I'm not sure if something that requires adding 10 casts (+ the ones in the setter) is an improvement. Casts are the thing that hides most bugs by far, since they disable almost all compiler warnings.

@nabijaczleweli
Copy link
Member

nabijaczleweli commented May 12, 2024

as far as i can tell, there's const-casts on both in the setter, merger, and deserialiser, all three of which are Envelope's member functions (or, as close as they can be), and entirely within the scope of the functions that should bypass the const.

there's an outlier in rfc2047_{de,de}code_envelope(struct Envelope *env), which juggles subject to conform to unfortunate memory management requirements and then calls the setter. also a member (or, sure, friend), also fine.

not sure where you're getting "cast away all the time" here, this reads to me as being perfectly as-designed (only Envelope members (and friends) are allowed to edit these directly) and as-desired (the shit works so everyone else is forced to use the setters so IMAP works)

@alejandro-colomar
Copy link
Member Author

alejandro-colomar commented May 12, 2024

as far as i can tell, there's const-casts on both in the setter, merger, and deserialiser, all three of which are Envelope's member functions (or, as close as they can be), and entirely within the scope of the functions that should bypass the const.

there's an outlier in rfc2047_{de,de}code_envelope(struct Envelope *env), which juggles subject to conform to unfortunate memory management requirements and then calls the setter. also a member (or, sure, friend), also fine.

Hmmm, okay.

not sure where you're getting "cast away all the time" here,

Just a way of speaking.

It's not all the time, but a non-trivial amount: 7/52

$ grep -rn 'real_subj\>' | pee 'wc -l > /dev/tty' 'cat' | grep '(char' | wc -l
52
7

this reads to me as being perfectly as-designed (only Envelope members (and friends) are allowed to edit these directly) and as-desired

Yup; just that the design implies some slightly dangerous stuff, IMHO. But I guess since it's your design, it's your prerogative.

(the shit works so everyone else is forced to use the setters so IMAP works)

I would have probably just not forced anything, and just document that the setters should be used. But I know that you prefer C++, so I guess, it's all right. At least now I understand why it's *const + casts.

And FTR, I would just remove IMAP from neomutt(1), but that' not going to happen. It would certainly simplify it a lot. :)

Thanks for explaining!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants