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

Refactor toir visit(StringExp) to avoid un-necessary computations. #3492

Merged
merged 10 commits into from
Jul 10, 2020

Conversation

looked-at-me
Copy link
Contributor

This doesn't include peekData() as described in #3490 not sure about the process about updating the interface files, if it's fine I'll just add it into this PR.

@kinke
Copy link
Member

kinke commented Jul 4, 2020

not sure about the process about updating the interface files

You could just add an inline method returning a DString, analogous to

ldc/dmd/expression.h

Lines 386 to 393 in 1494343

#if IN_LLVM
// The D version returns a slice.
DString peekString() const
{
assert(sz == 1);
return {len, static_cast<const char *>(string)};
}
#endif

@kinke
Copy link
Member

kinke commented Jul 6, 2020

Thx for figuring out the test failures. - Seems like the code duplication is pointless, so this should be further refactored. It doesn't have to be you, and it doesn't have to be in this PR, but it's ugly as hell once one knows about it.

@kinke
Copy link
Member

kinke commented Jul 9, 2020

Thx! I've pushed 2 more commits and am finally satisfied. ;)

@looked-at-me
Copy link
Contributor Author

LGTM, the failure is due to a timeout. Not sure if you want to run it again?

@kinke
Copy link
Member

kinke commented Jul 10, 2020

I've learned to accept unfortunate sporadic failures and CI hickups. [Plus I can't retrigger or cancel Travis jobs as I used to.]

In case you're wondering about the formatting - we use clang-format for a consistent C++ style.

@kinke kinke merged commit 1b5b405 into ldc-developers:master Jul 10, 2020
JohanEngelen added a commit to weka/ldc that referenced this pull request Oct 18, 2020
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