Conversation
Contributor
📝 WalkthroughWalkthroughThis PR makes widespread const-correctness and signature changes (several constructors and one method), upgrades multiple projects to C++20 with CompileAsCpp, adds Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
CefSharp.Core.Runtime/DragData.h (1)
29-37: Optional: considerconst CefRefPtr<CefDragData>&constructor parameter to eliminate the intermediate variable workaround.The constructor only reads from
dragData— it calls const member functions and copies the pointer into_wrappedDragData. Changing the parameter type would let all three call sites (Clone,Create,Image) pass the returned prvalue directly:♻️ Proposed refactor
- DragData(CefRefPtr<CefDragData> &dragData) : + DragData(const CefRefPtr<CefDragData> &dragData) :This is purely optional — the existing local-variable approach is correct and consistent with the broader pattern across the project.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CefSharp.Core.Runtime/DragData.h` around lines 29 - 37, The DragData constructor currently takes a non-const CefRefPtr<CefDragData>& causing call sites (Clone, Create, Image) to create an intermediate local to bind; change the constructor signature to accept const CefRefPtr<CefDragData>& dragData so prvalues can be passed directly, keep assignment to _wrappedDragData and the existing reads (IsReadOnly, GetFileName, IsFile, IsFragment, IsLink) unchanged, and update any call sites if necessary to remove the temporary-local workaround.CefSharp.Core.Runtime/Internals/CefContextMenuParamsWrapper.cpp (1)
114-114: Nit: redundant copy-initialization — prefer value-initialization shorthand.
= std::vector<CefString>()is equivalent to the default constructor and can be dropped:♻️ Proposed simplification
- std::vector<CefString> dictionarySuggestions = std::vector<CefString>(); + std::vector<CefString> dictionarySuggestions;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CefSharp.Core.Runtime/Internals/CefContextMenuParamsWrapper.cpp` at line 114, The variable dictionarySuggestions is being copy-initialized with an empty vector ("std::vector<CefString> dictionarySuggestions = std::vector<CefString>();") which is redundant; replace that with value-initialization such as "std::vector<CefString> dictionarySuggestions;" (or "std::vector<CefString> dictionarySuggestions{};") in CefContextMenuParamsWrapper.cpp to use the default constructor and remove the unnecessary copy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CefSharp.BrowserSubprocess.Core/CefSharp.BrowserSubprocess.Core.vcxproj`:
- Around line 114-115: The project now sets
<LanguageStandard>stdcpp20</LanguageStandard> with
<CompileAs>CompileAsCpp</CompileAs>, but /clr + C++20 requires a minimum Visual
Studio toolset and the optional C++/CLI component; update the project and repo
to enforce and document that requirement: add/msbuild-conditional checks or a
build-fail message in CefSharp.BrowserSubprocess.Core.vcxproj that validates the
MSVC toolset/version (v143 toolset with VS 2022 >= 17.10 recommended) and that
the "C++/CLI support for v143 build tools" component is available, update CI
images to a supported VS version (pin to v17.10+), and add a clear note to
CONTRIBUTING/README documenting the minimum Visual Studio version and required
C++/CLI component so local builds fail early and contributors know to install
the component.
---
Duplicate comments:
In
`@CefSharp.BrowserSubprocess.Core/CefSharp.BrowserSubprocess.Core.netcore.vcxproj`:
- Around line 157-158: Update all project configurations to consistently use
C++20 by setting the LanguageStandard element to "stdcpp20" and ensure CompileAs
is set to "CompileAsCpp" wherever C++ compilation is intended; apply this same
change for the new arm64 targets and the other configuration blocks that contain
the LanguageStandard and CompileAs elements so all 6 configurations match (look
for the LanguageStandard and CompileAs XML elements in the
CefSharp.BrowserSubprocess.Core.netcore.vcxproj and mirror the shown entries).
In `@CefSharp.Core.Runtime/CefSharp.Core.Runtime.netcore.vcxproj`:
- Around line 181-182: The LanguageStandard and CompileAs settings in
CefSharp.Core.Runtime.netcore.vcxproj should be set to stdcpp20 and CompileAsCpp
consistently across all configurations—ensure the same change is applied for
every occurrence referenced (also mirror the edits where LanguageStandard and
CompileAs appear) and confirm the identical update was made in
CefSharp.BrowserSubprocess.Core.vcxproj while noting the Visual Studio version
caveat mentioned in the review; verify the project elements <LanguageStandard>
and <CompileAs> are uniformly updated across all six configurations.
In `@CefSharp.Core.Runtime/CefSharp.Core.Runtime.vcxproj`:
- Around line 133-134: The project file has been updated to use C++20 by setting
the LanguageStandard to stdcpp20 and CompileAs to CompileAsCpp; ensure these
tags (LanguageStandard and CompileAs) are consistently updated across all
configurations by applying the same changes to the other occurrences referenced
(the blocks corresponding to the other configurations where the same tags
appear) so every configuration uses stdcpp20 and CompileAsCpp.
---
Nitpick comments:
In `@CefSharp.Core.Runtime/DragData.h`:
- Around line 29-37: The DragData constructor currently takes a non-const
CefRefPtr<CefDragData>& causing call sites (Clone, Create, Image) to create an
intermediate local to bind; change the constructor signature to accept const
CefRefPtr<CefDragData>& dragData so prvalues can be passed directly, keep
assignment to _wrappedDragData and the existing reads (IsReadOnly, GetFileName,
IsFile, IsFragment, IsLink) unchanged, and update any call sites if necessary to
remove the temporary-local workaround.
In `@CefSharp.Core.Runtime/Internals/CefContextMenuParamsWrapper.cpp`:
- Line 114: The variable dictionarySuggestions is being copy-initialized with an
empty vector ("std::vector<CefString> dictionarySuggestions =
std::vector<CefString>();") which is redundant; replace that with
value-initialization such as "std::vector<CefString> dictionarySuggestions;" (or
"std::vector<CefString> dictionarySuggestions{};") in
CefContextMenuParamsWrapper.cpp to use the default constructor and remove the
unnecessary copy.
|
✅ Build CefSharp 145.0.240-CI5418 completed (commit 82b3e9702d by @amaitland) |
amaitland
force-pushed
the
upgrade/cpp20
branch
from
8aae7e7 to
1a0b925
Compare
|
❌ Build CefSharp 145.0.240-CI5420 failed (commit 4062d3805c by @amaitland) |
amaitland
force-pushed
the
upgrade/cpp20
branch
from
1a0b925 to
84c1873
Compare
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
CefSharp.Core.Runtime/RequestContext.h (1)
45-49: Const-correctness change is correct; minor init-style inconsistency with sibling wrappers.All peer wrappers updated in this PR (e.g.,
CefBrowserWrapper,Browser,Frame,CefBrowserHostWrapper) use a member initializer list (: _member(param)), butRequestContextassigns in the body via_requestContext = context.get(). Both are equivalent, but for consistency you may consider aligning to the member-init pattern used elsewhere:♻️ Optional alignment to member init-list style
- RequestContext(const CefRefPtr<CefRequestContext>& context) - { - _requestContext = context.get(); - _settings = nullptr; - } + RequestContext(const CefRefPtr<CefRequestContext>& context) + : _requestContext(context), _settings(nullptr) + { + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CefSharp.Core.Runtime/RequestContext.h` around lines 45 - 49, The RequestContext constructor currently assigns members in the body; change it to use a member initializer list for consistency with other wrappers by initializing _requestContext with context.get() and _settings with nullptr in the constructor initializer list (i.e., update the RequestContext(const CefRefPtr<CefRequestContext>& context) constructor to initialize _requestContext and _settings via the member initializer list).CefSharp.Core.Runtime/Internals/CefBrowserWrapper.h (1)
23-26: LGTM.The
CefSharp.BrowserSubprocess.Core/CefBrowserWrapper.h(a separate class shown in the relevant snippets, not part of this PR) still takesCefRefPtr<CefBrowser>by value. Passing by value is already const-compatible so it's not broken, but if you want to unify the style across all wrapper constructors, that file is the one remaining outlier.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CefSharp.Core.Runtime/Internals/CefBrowserWrapper.h` around lines 23 - 26, Constructor signature in CefSharp.BrowserSubprocess.Core/CefBrowserWrapper.h currently takes CefRefPtr<CefBrowser> by value; change it to take a const CefRefPtr<CefBrowser>& to match the style used elsewhere. Update the CefBrowserWrapper(const CefRefPtr<CefBrowser> &browser) declaration/definition to use a const reference, ensure the initializer list still assigns _browser(browser) and that no call sites rely on copy semantics, and rebuild to verify no warnings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CefSharp.Core.Runtime/CefSharp.Core.Runtime.netcore.vcxproj`:
- Around line 181-182: Update the project documentation to explicitly require
Visual Studio 2022 version 17.6 or newer: mention in README and CONTRIBUTING
that due to use of C++20 in libcef_dll_wrapper and related headers (see
CefSharp.Core.Runtime.netcore.vcxproj using
<LanguageStandard>stdcpp20</LanguageStandard> together with
<CLRSupport>NetCore</CLRSupport>), VS 2022 17.6+ is the minimum toolset because
earlier VS2022 releases silently downgrade /std:c++20 to /std:c++17 under /clr;
also add a short note about the two C++/CLI limitations (managed-template
two-phase lookup and module imports under /clr) so integrators are aware.
---
Nitpick comments:
In `@CefSharp.Core.Runtime/Internals/CefBrowserWrapper.h`:
- Around line 23-26: Constructor signature in
CefSharp.BrowserSubprocess.Core/CefBrowserWrapper.h currently takes
CefRefPtr<CefBrowser> by value; change it to take a const CefRefPtr<CefBrowser>&
to match the style used elsewhere. Update the CefBrowserWrapper(const
CefRefPtr<CefBrowser> &browser) declaration/definition to use a const reference,
ensure the initializer list still assigns _browser(browser) and that no call
sites rely on copy semantics, and rebuild to verify no warnings.
In `@CefSharp.Core.Runtime/RequestContext.h`:
- Around line 45-49: The RequestContext constructor currently assigns members in
the body; change it to use a member initializer list for consistency with other
wrappers by initializing _requestContext with context.get() and _settings with
nullptr in the constructor initializer list (i.e., update the
RequestContext(const CefRefPtr<CefRequestContext>& context) constructor to
initialize _requestContext and _settings via the member initializer list).
|
✅ Build CefSharp 145.0.240-CI5424 completed (commit accc0afa54 by @amaitland) |
- Set NOMINMAX - Change compiler options - Fix compiler errors
amaitland
force-pushed
the
upgrade/cpp20
branch
from
dd4a067 to
e2b4d38
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
CefSharp.Core.Runtime/Internals/CefContextMenuParamsWrapper.cpp (1)
115-117: Consider removing the unusedresultvariable.The return value of
GetDictionarySuggestionsis captured but never checked. While the behavior is safe (an emptydictionarySuggestionsvector passed toStringUtils::ToClrreturns an empty list), the unused variable will likely trigger MSVC warning C4189 under the new C++20/CompileAsCppbuild settings.♻️ Proposed cleanup
- bool result = _wrappedInfo->GetDictionarySuggestions(dictionarySuggestions); + _wrappedInfo->GetDictionarySuggestions(dictionarySuggestions);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CefSharp.Core.Runtime/Internals/CefContextMenuParamsWrapper.cpp` around lines 115 - 117, Remove the unused local variable `result`: call `_wrappedInfo->GetDictionarySuggestions(dictionarySuggestions);` without assigning to `result`, then return `StringUtils::ToClr(dictionarySuggestions);` so the code still fills `dictionarySuggestions` but avoids the unused-variable warning in the CefContextMenuParamsWrapper method that contains the `GetDictionarySuggestions` call.CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.h (1)
88-88: Considerconst CefString&overCefStringby value.The non-const reference was presumably changed to by-value to fix C++20 compilation (temporaries can't bind to
T&). The idiomatic fix for a read-only string-like parameter isconst CefString&, which accepts both lvalues and rvalues without making a copy —CefStringinternally heap-allocates its string data, so the by-value form introduces an avoidable allocation on every call.♻️ Proposed refactor
- JavascriptRootObjectWrapper^ GetJsRootObjectWrapper(int browserId, CefString frameId); + JavascriptRootObjectWrapper^ GetJsRootObjectWrapper(int browserId, const CefString& frameId);The same change should be mirrored in
CefAppUnmanagedWrapper.cpp.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.h` at line 88, Change the GetJsRootObjectWrapper signature to accept the string by const reference (use const CefString& frameId) instead of by-value to avoid unnecessary allocations; update the corresponding implementation in CefAppUnmanagedWrapper.cpp to match the new signature and ensure any call sites still compile (they will accept lvalues and rvalues), and adjust any parameter usage in the GetJsRootObjectWrapper method body to treat frameId as a const reference.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@CefSharp.BrowserSubprocess.Core/CefSharp.BrowserSubprocess.Core.netcore.vcxproj`:
- Around line 157-158: Update project/docs to document that LanguageStandard set
to "stdcpp20" with CompileAs set to "CompileAsCpp" requires Visual Studio 2022
v17.6+ when used with /clr:netcore (older compilers silently fall back to
C++17), and note the mandatory C++/CLI support for v143 build tools optional
component plus the known C++20 limitations (two-phase name lookup for managed
templates and module import unsupported under C++/CLI). Add these details to
README and CONTRIBUTING, and update CI (AppVeyor image) to pin to VS 2022
v17.10+ (or minimally v17.6+) so all configurations using
LanguageStandard/CompileAs reflect this requirement.
---
Duplicate comments:
In `@CefSharp.BrowserSubprocess.Core/CefSharp.BrowserSubprocess.Core.vcxproj`:
- Around line 114-115: The project currently unconditionally sets
LanguageStandard to "stdcpp20" and CompileAs to "CompileAsCpp" which can break
builds on older VS/toolsets or when C++/CLI (/clr) component is used; update the
project file to guard or document this requirement: add a conditional check or
MSBuild property that gates setting LanguageStandard="stdcpp20" and
CompileAs="CompileAsCpp" on the toolset/version (VS 2022 v17.6+ or newer) or on
an explicit opt-in property, and if /clr or C++/CLI is enabled ensure the
project does not force CompileAsCpp in that configuration and emits a clear
error/message instructing users to upgrade to the required toolset or install
the C++/CLI component; reference the LanguageStandard, CompileAs, stdcpp20,
CompileAsCpp and /clr symbols when making the change.
---
Nitpick comments:
In `@CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.h`:
- Line 88: Change the GetJsRootObjectWrapper signature to accept the string by
const reference (use const CefString& frameId) instead of by-value to avoid
unnecessary allocations; update the corresponding implementation in
CefAppUnmanagedWrapper.cpp to match the new signature and ensure any call sites
still compile (they will accept lvalues and rvalues), and adjust any parameter
usage in the GetJsRootObjectWrapper method body to treat frameId as a const
reference.
In `@CefSharp.Core.Runtime/Internals/CefContextMenuParamsWrapper.cpp`:
- Around line 115-117: Remove the unused local variable `result`: call
`_wrappedInfo->GetDictionarySuggestions(dictionarySuggestions);` without
assigning to `result`, then return `StringUtils::ToClr(dictionarySuggestions);`
so the code still fills `dictionarySuggestions` but avoids the unused-variable
warning in the CefContextMenuParamsWrapper method that contains the
`GetDictionarySuggestions` call.
CefSharp.BrowserSubprocess.Core/CefSharp.BrowserSubprocess.Core.netcore.vcxproj
Show resolved
Hide resolved
|
✅ Build CefSharp 145.0.240-CI5426 completed (commit 249cc66f1e by @amaitland) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary:
CEF now requires C++20
Changes: [specify the structures changed]
How Has This Been Tested?
NA
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Refactor