Skip to content

optimized std::stack usage in simplecpp::TokenList::combineOperators()#659

Open
firewave wants to merge 2 commits into
cppcheck-opensource:masterfrom
firewave:combine
Open

optimized std::stack usage in simplecpp::TokenList::combineOperators()#659
firewave wants to merge 2 commits into
cppcheck-opensource:masterfrom
firewave:combine

Conversation

@firewave

@firewave firewave commented Jun 10, 2026

Copy link
Copy Markdown
Collaborator
  • use std::vector as underlying container for std::stack
  • initialize std::stack with initial value instead of inserting it

@firewave

firewave commented Jun 10, 2026

Copy link
Copy Markdown
Collaborator Author

This cuts the Ir for the function in question in half.

Running ./testrunner in valgrind:

GCC 16 - 73,545,066 -> 73,089,626 -> 72,638,322
Clang 22 - 71,260,479 -> 70,736,809 -> 70,321,207

Comment thread simplecpp.cpp
{
std::stack<bool> executableScope;
executableScope.push(false);
std::stack<bool, std::vector<bool>> executableScope{{false}};

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

isn't std::vector<bool> bad to use overall in c++11..
are you sure that all usage here will work fine..

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

As this is about performance and valgrind indicates that is fine in either compiler so I see no problem.

libc++ has been adding special handling for vector<bool> and we are also not using it as a bit field here (which is the common usage for that IIRC).

@danmar danmar Jun 27, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I am not talking about performance nor stuff that valgrind will detect. I referred to such problems:

  • No direct references: Because individual bits don't have memory addresses, std::vector::operator[] cannot return a bool&. Instead, it returns a temporary proxy object
  • Breaks generic code: If you write a template function expecting a T&, it will fail to compile if T is bool because a proxy object cannot be bound as a standard reference
  • Pointer failures: You cannot create a bool* pointing to elements inside a vector since a pointer requires an addressable byte.
  • Concealed auto-deduction issues: If you use auto& to deduce a boolean element, you will actually deduce the proxy object, not a boolean.
  • Thread safety pitfalls: Because multiple boolean elements share the same underlying byte or machine word, writing to different adjacent bits from different threads causes data races. They logically represent different indices, but they are physically the same memory block.
  • Incompatibility with Modern APIs: Modern C++ features like std::span cannot be used directly with vector because there is no contiguous bool* storage to point at.

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