Refactor expressionsem using debug condition

[12345swordy]

First step of implementing a log audit for dmd.
dlang/project-ideas#79

Rational: You should not have to modify the code in order to enable logging to see what is going on behind the scene when working in dmd. The logging system needs to be triggered by a compiler command when compiling dmd. The current logging code itself is inconstant as some of them rely on using version(none) while others are just marked as //printf. Using the debug condition allows this to be triggered by a compiler command. Which then can be documented and easy to use.

The first step is to wrap the printf statements in a debug condition statement. Then replace the printf statements with the log function that allows you to select the type of information that you would like to see by using the compiler flags that are created by the debug condition.

cc @RazvanN7

[dlang-bot]

Thanks for your pull request and interest in making D better, @12345swordy! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

• My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
• My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
• I have provided a detailed rationale explaining my changes
• New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

---

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#12636"

[thewilsonator]

otherwise looks good

[WalterBright]

I do not understand what the purpose of this change is. I spent a lot of time debugging expressionsem, and I never need to turn them on globally for dmd. If I did, it would absolutely bury me in irrelevant noise.

The idea is to turn them on in a focused, very targeted manner. There's no reasonable way to generalize this. The static if (LOGSEMANTIC) is trivially changed to static if (1 || LOGSEMANTIC) for a focused output, and change it back when done.

I'll also tend to turn on LOG-like variables (as enums) inside a function to turn on/off only the logging for that function. debug conditionals only act globally.

I don't see what value a log audit adds.

[RazvanN7]

@WalterBright You are the most knowledgeable person in terms of dmd internals, therefore whenever you want to debug something you know exactly where to look. People that are starting to contribute on the compiler or even experienced contributors sometimes find it hard to understand the chain of semantic analysis calls. For them, having a log file with tons of information is extremely helpful. I, personally, could have used something like this in the past. Also, this does not affect in any way your strategy of targeted printing, you can easily comment the debug statement.

One major advantage of the log system is that it could output a file that could be used as an input to a visual representation program that could create an image with the call chain.

[WalterBright]

debug is the wrong tool for a log system, as it is global to the file and not amenable to fine grained use. For example, look at escape.d. It uses static if and an enum argument set at function start to turn on/off a set of logging, not just one. This is very very handy.

Turn everything on, and the log file will be an utterly hopeless mass of useless data.

Printing the call chain is indeed highly useful, and the debugger does that nicely, without all the mess.

[WalterBright]

Needs a better rationale

[dkorpel]

The idea is to turn them on in a focused, very targeted manner. There's no reasonable way to generalize this. The static if (LOGSEMANTIC) is trivially changed to static if (1 || LOGSEMANTIC) for a focused output, and change it back when done.

You can just as well do:

//debug(LOGSEMANTIC)
{
    printf(...);
}

[dkorpel]

debug is the wrong tool for a log system, as it is global to the file and not amenable to fine grained use.

debug allows you to print to the console in pure functions. I know dmd rarely uses pure currently so calling printf is possible almost anywhere, but using debug for logging makes a lot of sense.

[dkorpel]

For example, look at escape.d. It uses static if and an enum argument set at function start to turn on/off a set of logging, not just one. This is very very handy.

I recently tried to figure out escape.d to fix some -dip1000 bugs, so I turned logging on. To do that, you have to do a mix of:

• change log = false to log = true
• change //printf(...) to printf(...)
• replace version(none) with version(all)

Afterwards, you go through your git diff to undo all that. I don't think it's handy currently.

On top of that, enums are often printed as numbers:

printf("storage class = x%llx\n", v.storage_class);

To read that out, you need to e.g. look up that STC.scope_ equals (1L << 19) and translate that to 0x80000 and then see if that fits with the number printed out.

Turn everything on, and the log file will be an utterly hopeless mass of useless data.

I agree, but a dedicated logging function can retrieve __FUNCTION__ and __MODULE__ via default parameters and filter based on that, and it can be turned on and off dynamically. For example, it would be very useful to temporarily turn logging off when object.d gets imported, which often clutters the log.

I know this PR alone does not yet show clear benefits, but I hope at least you're not opposed to dlang/project-ideas#79 as a whole. Maybe @12345swordy can outline the bigger picture a bit more, since this PR alone indeed lacks rationale.

[ibuclaw]

Having a skim over the project description, this goal seems misplaced.

Replace printf statements with the log audit statements that is generated only for debug mode, or passing a compiler option.

It would be better to just leave the existing printfs alone.

[12345swordy]

It would be better to just leave the existing printfs alone.

No, we need to replace the printfs, as the failing test suit relies on the console output to indicated that it passes or not. If you are running the failing test on your local machine and you want to know why the failing test are not passing than you do not want printf effecting it.
Using printf just creates unnecessary clutter on the console when it should be on it own console/file, so that you can focus on what matter. Better yet, you can use the file to generate better visual representation of what is going on behind the scene.

[12345swordy]

Needs a better rationale

Done.

[ibuclaw]

It would be better to just leave the existing printfs alone.

No, we need to replace the printfs, as the failing test suit relies on the console output to indicated that it passes or not. If you are running the failing test on your local machine and you want to know why the failing test are not passing than you do not want printf effecting it.
Using printf just creates unnecessary clutter on the console when it should be on it own console/file, so that you can focus on what matter. Better yet, you can use the file to generate better visual representation of what is going on behind the scene.

The printfs are not causing any harm to those who make use of them. Meanwhile a logging/audit system can be added independently alongside them.

[12345swordy]

It would be better to just leave the existing printfs alone.

No, we need to replace the printfs, as the failing test suit relies on the console output to indicated that it passes or not. If you are running the failing test on your local machine and you want to know why the failing test are not passing than you do not want printf effecting it.
Using printf just creates unnecessary clutter on the console when it should be on it own console/file, so that you can focus on what matter. Better yet, you can use the file to generate better visual representation of what is going on behind the scene.

The printfs are not causing any harm to those who make use of them. Meanwhile a logging/audit system can be added independently alongside them.

Sure thing. However we shouldn't allow any comment code in dmd, including printf.

[RazvanN7]

@WalterBright Can you please take another look at this?

[12345swordy]

@RazvanN7 I add further justification to this PR.

[RazvanN7]

cc @WalterBright

[ibuclaw]

Rationale does not address the critique of the change. The addition of a log/audit does not equate to changing the existing code used for occasional debugging. Particularly if the intermediate state has more drawbacks than the original.

[12345swordy]

Rationale does not address the critique of the change.

Yes it does. I said it right here:
"The current log audit code itself is inconstant as some of them rely on using version(none) while others are just marked as //printf. Using the debug condition allows this to be triggered by a compiler command. Which then can be documented and easy to use."
You should NOT be modifying code in order to enable logging in order figure out what is going on behind the compiler. As @dkorpel pointed out, you have to do several things in order to turn logging on.

The addition of a log/audit does not equate to changing the existing code used for occasional debugging.

It does if you want to log audit to

• log while running the failing test which is determine by the console output by the compiler.
• output information into a file format that involves importing the Phobos library.
• run certain Type of information that is determine by the newly created compiler flags.

This can not be achieved with the current system.

Particularly if the intermediate state has more drawbacks than the original.

You can always do this

//debug(LOGSEMANTIC)
{
    printf(...);
}

or this

enum LOGSEMANTIC = true;

This isn't any different than what is currently set up.

[12345swordy]

Let me provide wider context of why I am pursuing the audit log:
The debug integration in visual studio when it comes to D is very poor. Even with the bug fixes that LDC folks did, it is still very poor. It can not "look up" what the parameterless function is returning in the debug window without having it to call it directly in the code, and the information that is returning is very limited to down right straight up cryptic. The debug experience in d is 3/4th class compare to C#.

[ibuclaw]

This statement from @WalterBright.

debug is the wrong tool for a log system, as it is global to the file and not amenable to fine grained use.

I agree! This should be reconsidered so that a better approach is taken.

[12345swordy]

I agree! This should be reconsidered so that a better approach is taken.

What alternatives do you propose here then?

Using debug condition is intuitive and makes sense as you should be developing dmd in debug mode, not in release mode. The current system is unacceptable as it requires you to modify the code when you want information from dmd while debugging and you have to unmodified the code when you create a commit, so that it won't appear in the release build. Debug condition for all intents and purposes, allows you to "Break the rules", which is a good idea when it comes to logging. You can't break the rules, when using static if, as far as I know.

Any alternative that involves the programmer modifying the code to activate/modifying the logging system and not by a compiler flags is not a viable nor feasible alternative here.

[WalterBright]

is not a viable nor feasible alternative

I've been doing it for 40 years.

[12345swordy]

is not a viable nor feasible alternative

I've been doing it for 40 years.

The logging is for other people who are not familiar with dmd intervals as you are here. I wouldn't be doing this if the debug integration with VS isn't very subpar compare to other languages.

[WalterBright]

integration with VS

I didn't realize this had anything to do with VS. Can you explain further?

[12345swordy]

integration with VS

I didn't realize this had anything to do with VS. Can you explain further?

I already did here

[12345swordy]

Closing this branch as we don't see eye to eye on this.