Skip to content

Don't try to convert string if it's already an LLWString; also remove orphaned method declarations #2732

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Don't try to convert string if it's already an LLWString; also remove orphaned method declarations #2732

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 6 additions & 7 deletions indra/llui/lltexteditor.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,6 @@ class LLTextEditor :
const LLUUID& getSourceID() const { return mSourceID; }

const LLTextSegmentPtr getPreviousSegment() const;
const LLTextSegmentPtr getLastSegment() const;
void getSelectedSegments(segment_vec_t& segments) const;

void setShowContextMenu(bool show) { mShowContextMenu = show; }
Expand All @@ -217,8 +216,6 @@ class LLTextEditor :
void showContextMenu(S32 x, S32 y);
void drawPreeditMarker();

void assignEmbedded(const std::string &s);

void removeCharOrTab();

void indentSelectedLines( S32 spaces );
Expand All @@ -238,7 +235,6 @@ class LLTextEditor :

void autoIndent();

void findEmbeddedItemSegments(S32 start, S32 end);
void getSegmentsInRange(segment_vec_t& segments, S32 start, S32 end, bool include_partial) const;

virtual llwchar pasteEmbeddedItem(llwchar ext_char) { return ext_char; }
Expand Down Expand Up @@ -307,15 +303,18 @@ class LLTextEditor :
// Methods
//
void pasteHelper(bool is_primary);
void cleanStringForPaste(LLWString & clean_string);
void cleanStringForPaste(LLWString& clean_string);

public:
template <typename STRINGTYPE>
void pasteTextWithLinebreaks(const STRINGTYPE& clean_string)
{
pasteTextWithLinebreaksImpl(ll_convert(clean_string));
if constexpr (std::is_same_v<STRINGTYPE, LLWString>)
pasteTextWithLinebreaksImpl(clean_string);
else
pasteTextWithLinebreaksImpl(ll_convert(clean_string));
Copy link
Contributor

Choose a reason for hiding this comment

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

Certainly we want ll_convert<T>(const T& in) to be a no-op. Do you observe otherwise?

The design intent is to be able to use ll_convert() to adapt any conventional viewer string type to the intended target type, without having to break out special cases -- even if the source type is already the target type.

Copy link
Contributor Author

@Ansariel Ansariel Sep 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

You are still having to call the operator and shuffle the strings around - unless the compiler somehow optimizes it away. This change prevents that by doing the check at compile time and omitting the no-op call of the operator.

Copy link
Contributor

Choose a reason for hiding this comment

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

The key question seems to be whether there's zero runtime cost to a same-type ll_convert(). If that turns out not to be true, my preferred fix would be to try to eliminate that runtime cost.

I'm happy we now have if constexpr; I'd hate to have to use std::enable_if tricks to effect the same thing. My problem with the proposed change is that it suggests propagating the same logic everywhere the source might be the same type as the target. I'd rather have a 10-character function call than a 4-line idiom.

}
void pasteTextWithLinebreaksImpl(const LLWString & clean_string);
void pasteTextWithLinebreaksImpl(const LLWString& clean_string);

private:
void onKeyStroke();
Expand Down
Loading