Conversation
Mauller
added
Enhancement
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. WalkthroughThe pull request adds a legacy compiler implementation of ChangesAtomic Compatibility
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Feature Merge Risk: ⚪ Minimal · up to The identified VC6 compatibility and short-operator risks are addressed at the current head. No actionable issue remains from this review. 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation Issue Resolution Add the required operating-system-independent increment and decrement helper APIs. Add the VC6 assembly implementations for the required 16-bit and 64-bit operations. Add automated tests for the helper behavior and the VC6 compatibility path. Comment |
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 8b7c73e7-3012-4741-b690-2537e67b3954
📒 Files selected for processing (1)
Dependencies/Utility/Utility/atomic_compat.h
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.
c746f90 to
3d80556
Compare
3d80556 to
240f4da
Compare
|
@coderabbitai PR updated |
240f4da to
c75098e
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Dependencies/Utility/Utility/atomic_compat.h (1)
44-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
memory_orderparameters to keep the two branches source compatible.The VC6 specializations expose
load(),store(),exchange(),fetch_*()andcompare_exchange_*()without a memory-order argument. Shared code that compiles on both branches cannot writeflag.load(std::memory_order_acquire)orcounter.fetch_add(1, std::memory_order_relaxed), because the standard branch accepts those calls and the VC6 branch rejects them. The interlocked functions are already full barriers, so the parameter can be ignored.Declare a
memory_orderenum in the VC6 branch and accept a defaulted, unused parameter in each operation.♻️ Sketch of the compatible signatures
namespace std { + enum memory_order + { + memory_order_relaxed, + memory_order_consume, + memory_order_acquire, + memory_order_release, + memory_order_acq_rel, + memory_order_seq_cst + }; + // Capture unsupported types and error during compilation template<class T> class atomic;- long load() const + long load(memory_order = memory_order_seq_cst) const { return InterlockedCompareExchange(&value_, 0, 0); } - void store(long value) + void store(long value, memory_order = memory_order_seq_cst) { InterlockedExchange(&value_, value); }Apply the same pattern to the
short,intandboolspecializations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 20f56eae-b2e9-450d-8ceb-14e570fc10ed
📒 Files selected for processing (2)
Dependencies/Utility/Utility/atomic_compat.hDependencies/Utility/Utility/interlocked_adapter.h
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.
|
@greptile can you update your review as it is invalid now, VC6 interlocked calls do not fail as they are called through the wrapper functions in |
|
|
Caball009
left a comment
Caball009
left a comment
There was a problem hiding this comment.
Incomplete review; I haven't paid attention to the complexities of <short>.
I assume the lack of support for unsigned integers is because the win32 atomic functions use LONG everywhere.
c75098e to
040d75b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 7cdb97ac-d4ac-4dcb-8b00-9b81a533e1f8
📒 Files selected for processing (1)
Dependencies/Utility/Utility/atomic_compat.h
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.
c5d2c7e to
55b08e5
Compare
xezon
left a comment
xezon
left a comment
There was a problem hiding this comment.
It would be nice if this was tested for correctness side by side with real std::atomic. Claude et al could do that.
55b08e5 to
40fdb4d
Compare
| { | ||
| // Capture unsupported types and error during compilation | ||
| template<class T> | ||
| class atomic; |
There was a problem hiding this comment.
I don't think it is necessary to make 4 template specializations. Just make 1 template implementation, with encode and decode, which is no-op for long, and identical for all else.
There was a problem hiding this comment.
I can take a look, but bool needs to have a separate implementation as there are numerical operations that make no sense on bool and don't exist on it in the STD implementation. Also there are logical operation on bool that don't exist or make no sense on numeric types.
There was a problem hiding this comment.
You can specialize bool then when needed.
| #if !(defined(_MSC_VER) && _MSC_VER < 1300) || __cplusplus >= 201103L | ||
| #include <atomic> | ||
| #else | ||
| #include <windows.h> |
There was a problem hiding this comment.
Always put
#ifndef WIN32_LEAN_AND_MEAN
#define WIN32_LEAN_AND_MEAN
#endifbefore. Unless it interlocked functions only include without that.
There was a problem hiding this comment.
i could probably drop the windows.h include since it was there for the interlocked functions, but those are now included throught the interlocked adapter.
There was a problem hiding this comment.
The interlocked_adapter does not include windows.h
There was a problem hiding this comment.
Ah i might actually need to include this as it seems that not all places include the windows header that might end up with the atomic compat.
While testing with ascii string, i get errors during compilation that the windows specific types are unidentified within interlocked_adapter.
There was a problem hiding this comment.
Hmm, it actually fails to compile if i include
#ifndef WIN32_LEAN_AND_MEAN
#define WIN32_LEAN_AND_MEAN
#endif
before windows.h
There was a problem hiding this comment.
That seems not good. What if someone includes this file after defining WIN32_LEAN_AND_MEAN themselves? This basically happens everywhere in game.
There was a problem hiding this comment.
I have a change coming where all custom WIN32_LEAN_AND_MEAN defines are removed and it is just defined once in precompiled.h
40fdb4d to
d6740d0
Compare
|
Updated after doing the house keeping from the various comments. I will look at trying to make this a bit more generic next. |
| // This file contains VC6 compatible atomic classes for the long, int, short and bool types | ||
| #pragma once | ||
|
|
||
| #if !(defined(_MSC_VER) && _MSC_VER < 1300) || __cplusplus >= 201103L |
There was a problem hiding this comment.
How about we use __has_include. I did the same approach for usp10 in #3342
atomic_adapter.h:
#if defined(__has_include)
#if __has_include(<atomic>)
#define HAVE_ATOMIC_H 1
#endif
#endif
#ifdef HAVE_ATOMIC_H
#include <atomic.h>
#else
#include "atomic_compat.h"
#endif
d6740d0 to
fefbd23
Compare
|
Just a rebase on recent main before continuing work |
fefbd23 to
fd33e87
Compare
|
A little bit more LLM assistance but i think this one is most of the way there. The A lot of places had to use compare and store (CAS) loops to make sure the relevant operations happened in the right places to handle signed overflows and such. The various |
xezon
left a comment
xezon
left a comment
There was a problem hiding this comment.
Looks promising. I think the overall design is fine.
|
|
||
| T operator^=(T value) | ||
| { | ||
| T oldValue = fetch_xor(value); |
There was a problem hiding this comment.
added const to places where it's reasonable to do so
There was a problem hiding this comment.
Still a couple in the exchange loops that could be made const.
fd33e87 to
7edfbe0
Compare
|
Lots of tweaks based on feedback. |
7edfbe0 to
a49a732
Compare
|
Tweaked. I do test this by replacing |
a49a732 to
54e6a21
Compare
3 participants
Closes: #887
This PR implements a set of std atomic like VC6 template classes.
This will allow the use of
std::atomic<type>variable;in vc6 code, allowing the cleanup and replacement of legacy ASM in places such as the mutex classes.Internally the template classes use a
longvariable and the windowsinterlockedfunctions which only work with the long type.This will result in more memory usage in VC6 builds but will provide a cleaner interface and transition to
std::atomicin other compilers.Currently the only supported types, including their unsigned variants, are;
longintshort,charandboolwhere the bool type has a more limited interface similar to how it is implemented in the std library.Edit - This is designed to just work with base types where atomic reference counting or an atomic boolean is required.
Using atomic trivial custom classes will have to be done when VC6 is dropped.
The contents of this PR were initially templated using AI then fixed up and extended by a living munkee.
Edit - Further work with LLM assistance consolidated the template specialisations into a generic template with specialise atomic_trait structs that limit the supported types.
For the AI's reviewing, this is meant to be within the
stdnamespace to allow the use of the realstdlibrary outside of VC6 code. This is okay to do in VC6 builds as atomic does not exist within VC6 and C++98. This then allows a single implementation at the site of use that works directly with the std libraries atomic.