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

Proposal: Remove _Ptr(_Const) typedefs #2785

Closed
glebm opened this issue Dec 4, 2018 · 4 comments
Closed

Proposal: Remove _Ptr(_Const) typedefs #2785

glebm opened this issue Dec 4, 2018 · 4 comments

Comments

@glebm
Copy link
Contributor

glebm commented Dec 4, 2018

The libsass codebase uses typedefs for pointers and const pointers, such as Expression_Ptr.

However, pointers should be handled with care but it's easy to miss a pointer if it doesn't look like one.

@xzyfer @mgreter What do you think about removing the _Ptr and _Ptr_Const typedefs?

This can be done with the following command (GNU sed, for macOS and FreeBSD use sed -i ''):

find src \( -name '*.cpp' -o -name '*.hpp' \) -exec sed -i \
'/typedef.*_Ptr[_;]/d;'\
's/\([A-Za-z_]*\)_Ptr_Const/const \1*/g;'\
's/\([A-Za-z_]*\)_Ptr/\1*/g;'\
's/\([a-z]\)##\*/\1*/g' '{}' \+
@xzyfer
Copy link
Contributor

xzyfer commented Dec 4, 2018

I am fully in favour of removing these. I continually struggle to know what types I'm dealing with at any given time.

@glebm
Copy link
Contributor Author

glebm commented Dec 5, 2018

Great! I'm going to wait until @mgreter finishes #2557 to avoid merge conflicts.

@mgreter
Copy link
Contributor

mgreter commented Dec 14, 2018

I'm not really pro or con this change. My initial reason to introduce these types was to be able to check (search) which types I already converted during the conversion from global memory handling to reference counted memory handling. What I don't like are the Const_Ptr types, but I didn't find another solution to solve the issues at hand. And I guess it's rather cosmetics and personal choice what one prefers.

@xzyfer not sure I got your comment right, but the difference is pretty easy. *_Obj is something that is reference counted and will be deleted when it goes out of scope, a *_Ptr is just a pointer to that object and it will not automatically be cleaned up if it is not assigned to an *_Obj later on ...

@glebm
Copy link
Contributor Author

glebm commented Dec 14, 2018

2 pro, 1 neutral, let's do this.

@mgreter Is now a good time or would you like to finish the subweave PR first (to reduce conflicts)?

glebm added a commit to glebm/libsass that referenced this issue Dec 20, 2018
Removes these typedefs because pointers should be handled with care.
Having them visually stand out in the code helps us avoid common
pitfalls, such as accidentally using `Ptr` instead of `Obj`.

This change has been automatically generated using the following command:

```bash
find src \( -name '*.cpp' -o -name '*.hpp' \) -exec sed -i \
'/typedef.*_Ptr[_;]/d;'\
's/\([A-Za-z_]*\)_Ptr_Const/const \1*/g;'\
's/\([A-Za-z_]*\)_Ptr/\1*/g;'\
's/\([a-z]\)##\*/\1*/g' '{}' \+
```

Fixes sass#2785
glebm added a commit to glebm/libsass that referenced this issue Mar 19, 2019
Removes these typedefs because pointers should be handled with care.
Having them visually stand out in the code helps us avoid common pitfalls,
such as accidentally using `Ptr` instead of `Obj`.

This change has been automatically generated using the following command:

```bash
find src \( -name '*.cpp' -o -name '*.hpp' \) -exec sed -i \
'/typedef.*_Ptr[_;]/d;'\
's/\([A-Za-z_]*\)_Ptr_Const/const \1*/g;'\
's/\([A-Za-z_]*\)_Ptr/\1*/g;'\
's/\([a-z]\)##\*/\1*/g' '{}' \+
```

Fixes sass#2785
glebm added a commit to glebm/libsass that referenced this issue Mar 19, 2019
Removes these typedefs because pointers should be handled with care.
Having them visually stand out in the code helps us avoid common pitfalls,
such as accidentally using `Ptr` instead of `Obj`.

This change has been automatically generated using the following command:

```bash
find src \( -name '*.cpp' -o -name '*.hpp' \) -exec sed -i \
'/typedef.*_Ptr[_;]/d;'\
's/\([A-Za-z_]*\)_Ptr_Const/const \1*/g;'\
's/\([A-Za-z_]*\)_Ptr/\1*/g;'\
's/\([a-z]\)##\*/\1*/g' '{}' \+
```

Fixes sass#2785
xzyfer pushed a commit that referenced this issue Mar 20, 2019
Removes these typedefs because pointers should be handled with care.
Having them visually stand out in the code helps us avoid common pitfalls,
such as accidentally using `Ptr` instead of `Obj`.

This change has been automatically generated using the following command:

```bash
find src \( -name '*.cpp' -o -name '*.hpp' \) -exec sed -i \
'/typedef.*_Ptr[_;]/d;'\
's/\([A-Za-z_]*\)_Ptr_Const/const \1*/g;'\
's/\([A-Za-z_]*\)_Ptr/\1*/g;'\
's/\([a-z]\)##\*/\1*/g' '{}' \+
```

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

No branches or pull requests

3 participants